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
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
66 changes: 57 additions & 9 deletions google/cloud/spanner_dbapi/connection.py
Expand Up @@ -10,15 +10,12 @@
import warnings

from google.cloud import spanner_v1
from google.cloud.spanner_v1.pool import BurstyPool

from .cursor import Cursor
from .exceptions import InterfaceError

AUTOCOMMIT_MODE_WARNING = (
"This method is non-operational, as Cloud Spanner"
"DB API always works in `autocommit` mode."
"See https://github.com/googleapis/python-spanner-django#transaction-management-isnt-supported"
)
AUTOCOMMIT_MODE_WARNING = "This method is non-operational in autocommit mode"

ColumnDetails = namedtuple("column_details", ["null_ok", "spanner_type"])

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

self._pool.bind(database)

self.instance = instance
self.database = database
self.is_closed = False

self._ddl_statements = []
self.transactions = []
IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved

self.is_closed = False
self._autocommit = False
c24t marked this conversation as resolved.
Show resolved Hide resolved

@property
def autocommit(self):
"""Autocommit mode flag for this connection.

:rtype: bool
:returns: Autocommit mode flag value.
"""
return self._autocommit

@autocommit.setter
def autocommit(self, value):
"""Change this connection autocommit mode.

:type value: bool
:param value: New autocommit mode state.
"""
if value and not self._autocommit:
self.commit()
IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved

self._autocommit = value

def session_checkout(self):
"""Get a Cloud Spanner session.

:rtype: :class:`google.cloud.spanner_v1.session.Session`
:returns: Cloud Spanner session object ready to use.
"""
return self._pool.get()
IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved

def cursor(self):
self._raise_if_closed()
Expand Down Expand Up @@ -142,18 +174,34 @@ def get_table_column_schema(self, table_name):
def close(self):
"""Close this connection.

The connection will be unusable from this point forward.
The connection will be unusable from this point forward. Rollback
will be performed on all the pending transactions.
IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved
"""
for transaction in self.transactions:
transaction.rollback()

IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved
self.__dbhandle = None
self.is_closed = True

def commit(self):
"""Commit all the pending transactions."""
warnings.warn(AUTOCOMMIT_MODE_WARNING, UserWarning, stacklevel=2)
if self.autocommit:
warnings.warn(AUTOCOMMIT_MODE_WARNING, UserWarning, stacklevel=2)
c24t marked this conversation as resolved.
Show resolved Hide resolved
else:
for transaction in self.transactions:
transaction.commit()

self.transactions = []
IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved

def rollback(self):
"""Rollback all the pending transactions."""
warnings.warn(AUTOCOMMIT_MODE_WARNING, UserWarning, stacklevel=2)
if self.autocommit:
warnings.warn(AUTOCOMMIT_MODE_WARNING, UserWarning, stacklevel=2)
c24t marked this conversation as resolved.
Show resolved Hide resolved
else:
for transaction in self.transactions:
transaction.rollback()

self.transactions = []

def __enter__(self):
return self
Expand Down
15 changes: 15 additions & 0 deletions google/cloud/spanner_dbapi/cursor.py
Expand Up @@ -68,6 +68,7 @@ def __init__(self, connection):
self._connection = connection
self._is_closed = False

self.transaction = None
# the number of rows to fetch at a time with fetchmany()
self.arraysize = 1

Expand All @@ -88,6 +89,20 @@ def execute(self, sql, args=None):

self._res = None

if not self._connection.autocommit:
if (
not self.transaction
or self.transaction.committed
or self.transaction.rolled_back
):
self.transaction = self._connection.session_checkout().transaction()
IlyaFaer marked this conversation as resolved.
Show resolved Hide resolved
self.transaction.begin()
self._connection.transactions.append(self.transaction)

self._res = self.transaction.execute_sql(sql)
self._itr = PeekIterator(self._res)
return

# Classify whether this is a read-only SQL statement.
try:
classification = classify_stmt(sql)
Expand Down
3 changes: 2 additions & 1 deletion tests/spanner_dbapi/test_connection.py
Expand Up @@ -49,8 +49,9 @@ def test_close(self):
connection.cursor()

@mock.patch("warnings.warn")
def test_transaction_management_warnings(self, warn_mock):
def test_transaction_autocommit_warnings(self, warn_mock):
connection = self._make_connection()
connection.autocommit = True

connection.commit()
warn_mock.assert_called_with(
Expand Down