feat: support transactions management #535
Conversation
IlyaFaer
added
type: feature request
google-cla
bot
added
the
cla: yes
|
@olavloite, @c24t, I've written the base implementation for transactions management, PTAL. All four system tests (with key cases) are passing locally, though sometimes transactions are aborted (this is okay while we're not implemented an aborted transactions retry, I suppose). User interface is completely the same as in other Python databases, e.g. SQLite 3, so I suppose we're suiting PEP 249 completely. |
| @@ -37,11 +34,46 @@ class Connection: | |||
| """ | |||
|
|
|||
| def __init__(self, instance, database): | |||
| self._pool = BurstyPool() | |||
There was a problem hiding this comment.
Having a connection pool for each Connection is too much. One connection will never use more than one session at any time, as:
- A connection can execute at most one transaction at any time.
- A session can execute at most one (read/write) transaction at any time.
This is fine for a prototype, for production purposes, the implementation should do one of the following:
- Create one session pool globally that is reused for all connections that are opened. This is the preferable implementation, as the connections would benefit from all optimizations that might be done to the session pool implementation.
- OR: Open one session (not session pool) for each connection, and use that session for all operations on that connection. Conceptually, a Cloud Spanner session is quite comparable to a Python database connection. Implementing it this way would however mean that the connection needs to do all session management by itself, including things like keep-alive pings, handling the backend dropping a session, etc. It is therefore not recommended to do that, and instead rely on the session pool handling all that for you.
There was a problem hiding this comment.
Hm-m, as our common way to create connections is a constructor-function (according to PEP 249), which doesn't save any condition, it seems like we should initiate session pool for the whole package. I don't see big problems with that.
|
@IlyaFaer Thanks for the draft. This seems like a good first step towards transaction management. One important design decision to made here, is how we should handle sessions. Preferably, the Connection API should rely on the session pool implementation from the client library. That would prevent us from having to implement all session handling once more for connections. It does however mean that there must be some kind of global session pool that is reused by all connections that are opened. |
…n, return used sessions back to the pool
|
@skuruppu, @larkee, PTAL, transactions management seems to be more or less implemented. We still have some ongoing conversations with @olavloite, but I think it'll be good if you'll take a look as well, and evaluate the concepts (with Python expertize too). |
|
@olavloite, @c24t, I've re-read the JDBC design doc - it looks like only DDL statements should not be executed within transaction. Fixed this part and added args parsing (using the existing functions). I've also fixed emulator configurations, so system tests on emulator are now working fine. |
There was a problem hiding this comment.
A few comments, but it looks very good overall. I think this could reasonably be approved as-is, but @IlyaFaer I'll give you a chance to respond before I stamp it.
Also thanks @olavloite for the detailed review!
No description provided.