Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support transactions management #535

Merged
merged 17 commits into from
Oct 22, 2020

Conversation

IlyaFaer
Copy link
Contributor

No description provided.

@IlyaFaer IlyaFaer added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. api: spanner Issues related to the googleapis/python-spanner-django API. labels Oct 12, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 12, 2020
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
tests/system/test_system.py Show resolved Hide resolved
@IlyaFaer
Copy link
Contributor Author

IlyaFaer commented Oct 12, 2020

@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.

google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
tests/system/test_system.py Outdated Show resolved Hide resolved
@@ -37,11 +34,46 @@ class Connection:
"""

def __init__(self, instance, database):
self._pool = BurstyPool()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a connection pool for each Connection is too much. One connection will never use more than one session at any time, as:

  1. A connection can execute at most one transaction at any time.
  2. 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:

  1. 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.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/cursor.py Outdated Show resolved Hide resolved
@olavloite
Copy link
Contributor

@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.

google/cloud/spanner_dbapi/__init__.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/__init__.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
tests/system/test_system.py Outdated Show resolved Hide resolved
@IlyaFaer
Copy link
Contributor Author

@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).

@skuruppu skuruppu requested a review from larkee October 15, 2020 10:23
@IlyaFaer
Copy link
Contributor Author

IlyaFaer commented Oct 16, 2020

@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.

Copy link
Contributor

@c24t c24t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
google/cloud/spanner_dbapi/cursor.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
google/cloud/spanner_dbapi/__init__.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/__init__.py Outdated Show resolved Hide resolved
@IlyaFaer IlyaFaer marked this pull request as ready for review October 20, 2020 10:12
@IlyaFaer IlyaFaer requested a review from a team as a code owner October 20, 2020 10:12
google/cloud/spanner_dbapi/config.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be confusion about session pools and database binding. I've tried to explain best I can but let me know if you have further questions 👍

google/cloud/spanner_dbapi/connection.py Outdated Show resolved Hide resolved
google/cloud/spanner_dbapi/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@c24t c24t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @larkee any comments before merging this?

Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one question for the transaction_checkout method

google/cloud/spanner_dbapi/connection.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner-django API. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants