feat: support transactions management by IlyaFaer · Pull Request #535 · googleapis/python-spanner-django · GitHub

No description provided.

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

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.

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

…n-spanner-django into transactions_retry

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

…ion close

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!

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 👍

LGTM, @larkee any comments before merging this?

LGTM, just one question for the transaction_checkout method

…n-spanner-django into transactions_retry

IlyaFaer added 5 commits October 9, 2020 15:58

feat: implement transactions management

fb958f9

feat: transactions management implementation

4b7273f

add autocommit property setter and getter, add more system tests

44fd7dd

Merge branch 'master' into transactions_retry

3861550

fix unit tests

dc7e5a4

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 Bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 12, 2020

IlyaFaer commented Oct 12, 2020

View reviewed changes

Comment thread google/cloud/spanner_dbapi/connection.py

Comment thread google/cloud/spanner_dbapi/connection.py Outdated

Comment thread tests/system/test_system.py

olavloite reviewed Oct 13, 2020

View reviewed changes

use global sessions pool, use single transaction for single connectio…

75e0685

…n, return used sessions back to the pool

IlyaFaer commented Oct 13, 2020

View reviewed changes

Comment thread google/cloud/spanner_dbapi/__init__.py Outdated

olavloite reviewed Oct 13, 2020

View reviewed changes

Comment thread google/cloud/spanner_dbapi/__init__.py Outdated

Comment thread google/cloud/spanner_dbapi/__init__.py Outdated

Comment thread google/cloud/spanner_dbapi/connection.py Outdated

Comment thread tests/system/test_system.py Outdated

del excess license lines

0f69035

skuruppu requested a review from larkee October 15, 2020 10:23

mf2199 and others added 4 commits October 15, 2020 14:19

Merge branch 'master' into transactions_retry

2c24eac

don't run DDL in transactions, fix system tests on emulator

79666d7

Merge branch 'transactions_retry' of https://github.com/q-logic/pytho…

635ec33

…n-spanner-django into transactions_retry

fix emulator config

84d67fe

don't roll back transactions which are already rolled back on connect…

61e2af8

…ion close

c24t reviewed Oct 19, 2020

View reviewed changes

some fixes

efbdab3

IlyaFaer marked this pull request as ready for review October 20, 2020 10:12

IlyaFaer requested a review from a team October 20, 2020 10:12

mf2199 reviewed Oct 21, 2020

View reviewed changes

larkee suggested changes Oct 21, 2020

View reviewed changes

Comment thread google/cloud/spanner_dbapi/connection.py Outdated

Comment thread google/cloud/spanner_dbapi/__init__.py Outdated

IlyaFaer and others added 2 commits October 21, 2020 11:29

don't use global pool

5e23e59

Merge branch 'master' into transactions_retry

95a36b4

c24t approved these changes Oct 21, 2020

View reviewed changes

larkee reviewed Oct 22, 2020

View reviewed changes

Comment thread google/cloud/spanner_dbapi/connection.py

larkee approved these changes Oct 22, 2020

View reviewed changes

IlyaFaer added 2 commits October 22, 2020 10:57

add autocommit mode note

ba1df36

Merge branch 'transactions_retry' of https://github.com/q-logic/pytho…

f691dc9

…n-spanner-django into transactions_retry

c24t merged commit 2f2cd86 into googleapis:master Oct 22, 2020

IlyaFaer deleted the transactions_retry branch November 20, 2020 08:21

This was referenced Apr 17, 2024

chore(main): release 3.0.0 #869

Closed

chore(main): release 3.0.0 #875

Closed

chore(main): release 3.0.0 #877

Closed

chore(main): release 3.1.0 #878

Closed

chore(main): release 3.1.0 #881

Merged

                   """
                   def __init__(self, instance, database):
+                      self._pool = BurstyPool()

feat: support transactions management by IlyaFaer · Pull Request #535 · googleapis/python-spanner-django · GitHub

feat: support transactions management - #535

feat: support transactions management#535c24t merged 17 commits intogoogleapis:mastergoogleapis/python-spanner-django:masterfrom MaxxleLLC:transactions_retryMaxxleLLC/python-spanner-django:transactions_retryCopy head branch name to clipboard

Conversation

IlyaFaer commented Oct 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IlyaFaer commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olavloite Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

IlyaFaer Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

olavloite commented Oct 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IlyaFaer commented Oct 14, 2020

Uh oh!

IlyaFaer commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larkee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

larkee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feat: support transactions management#535
c24t merged 17 commits into
googleapis:masterfrom
MaxxleLLC:transactions_retry

IlyaFaer commented Oct 12, 2020 •
edited
Loading

IlyaFaer commented Oct 16, 2020 •
edited
Loading