Implement multi-use snapshots by tseaver · Pull Request #3615 · googleapis/google-cloud-python

tseaver · 2017-07-17T18:38:33Z

Multi-use snapshots trigger an "implicit" server-side transaction, and capture its ID on the first request. Subsequent requests return that ID, allowing for isolation from other changes. We default to multi_use=False because that mode is much more performant for the simple case.

This feature is one which we originally decided to leave out, but the P0 system test list requires that it be implemented.

I think that a commitwise review might be easier than reviewing the whole enchilada.

dhermes

No real issues, just cosmetics.

Sorry for the delay in review.

Sign in to view


- def snapshot(self, read_timestamp=None, min_read_timestamp=None,
- max_staleness=None, exact_staleness=None):
+ def snapshot(self, **kw):


Sign in to view

 if len(flagged) > 1:
 raise ValueError("Supply zero or one options.")

+ if multi_use and (min_read_timestamp or max_staleness):


Sign in to view


+ if multi_use and (min_read_timestamp or max_staleness):
+ raise ValueError(
+ "'multi_use' is incompatile with "


Sign in to view

 self._resume_token = response.resume_token

 if self._metadata is None: # first response
+ # XXX: copy implicit txn ID to snapshot, if present.


Sign in to view

 self.assertEqual(options.kwargs['metadata'],
 [('google-cloud-resource-prefix', database.name)])

+ def test_execute_sql_wo_mulit_use(self):


Sign in to view

 return Value(list_value=_make_list_value_pb(values))

+ @staticmethod
+ def _makeResultSetMetadata(fields=(), transaction_id=None):


Sign in to view

 # limitations under the License.


+import mock


dhermes

@tseaver Can you stop adding system tests to this PR? They keep coming in "after review" (e.g. 7d6fa02)

Sign in to view

+
+ def test_multiuse_snapshot_read_isolation_exact_staleness(self):
+ import time
+ from datetime import timedelta


tseaver · 2017-07-19T23:15:40Z

Can you stop adding system tests to this PR? They keep coming in "after review"

I haven't added any: I had to rebase to fix the conflicts with your assertIs PR.

Sign in to view

+ return StreamedResultSet(iterator)

 def execute_sql(self, sql, params=None, param_types=None, query_mode=None,
 resume_token=b''):


Sign in to view

 ``exact_staleness`` old.
+
+ :type multi_use: :class:`bool`
+ :param multi_use: If true, the first read operation creates a read-only


Sign in to view

+ if self._multi_use:
+ return StreamedResultSet(iterator, source=self)
+ else:
+ return StreamedResultSet(iterator)


vkedia · 2017-07-20T01:09:40Z

The documentation on this page suggests that snapshot already allows you to do multiple reads at a consistent snapshot.
This is what it says:

A Snapshot represents a read-only transaction: when multiple read operations are peformed via a Snapshot, the results are consistent as of a particular point in time.

In light of this PR that is clearly incorrect.

@jonparrott We also claim the same thing in our [sample code])
https://cloud.google.com/spanner/docs/getting-started/python/#retrieve_data_using_read-only_transactions). That too is incorrect.

@lukesneeringer @bjwatson Can we please get this merged asap. This is a severe bug.

lukesneeringer

Seeing no actual concerns (either in my own review or the other comments), this is good to go out.

vkedia · 2017-07-20T18:13:56Z

@lukesneeringer I believe my comments need to be addressed before merging this. Specifically
#3615 (comment)

lukesneeringer · 2017-07-20T18:27:56Z

Ah -- confirmed. Thanks @vkedia. (@tseaver His proposed solution seems reasonable to me. Thoughts?)

vkedia · 2017-07-21T17:27:29Z

@tseaver Any updates on this?

tseaver · 2017-07-24T17:12:15Z

I can see adding an explicit Snapshot.begin to capture a transaction ID.

In addition, I had thought of having a _transaction_pending flag on the multi-use snapshot, which would cause subsequent read / execute_sql requests to raise an exception. This would smooth the "garden path" usage (i.e., reading / querying with fetch, rather than interleaving).

- Convert 'Database.snapshot' and 'Session.snapshot' factories to take / forward '**kw'.

- When reading / executing SQL for a multi-use snapshot, pass the snapshot as the iterator's source.

- PartialResultSet - ResultSetMetadata - ResultSetStats.

- Source will only be set for multi-use snapshots.

tseaver · 2017-07-24T18:13:47Z

To implement the single-use request guard, the snapshot needs to maintain a request counter, which we can use to detect the "interleaved" case.

vkedia · 2017-07-24T18:14:14Z

That sounds good. We can have an explicit begin which users can chose to call to get a transaction id and then they can interleave reads. If they do not call begin then the first read or query will create the transaction and if they try to interleave before it has returned they will get back an error.
So essentially users can interleave reads only after there is a transaction id which can be got by either calling begin or by the first read

vkedia · 2017-07-24T18:21:42Z

Actually why do we need Snapshot to support single_use? For single use transactions database.read and database.query method should be sufficient. We should extend those methods to also take timestamp bound specific options.

tseaver · 2017-07-24T18:44:47Z

Database.read and Database.execute_sql create Snapshot instances under the covers (via Session.read / Session.execute_sql). I just opened #3659 to track adding the read isolation parameters to those methods.

tseaver · 2017-07-24T19:19:42Z

@vkedia I've just updated the system tests to pass multi_use=True where needed (I also fixed a new bug where Transaction was not implicitly multi-use for reads). I am, however, seeing system failures on the third invocation using multi-use snapshots:

________________TestSessionAPI.test_execute_sql_w_query_param_________________Traceback (mostrecentcalllast):
File"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/tests/system/test_system.py", line960, intest_execute_sql_w_query_paramexpected=[(19,), (99,)],
File"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/tests/system/test_system.py", line888, in_check_sql_resultssql, params=params, param_types=param_types))
File"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/.nox/sys-3-6/lib/python3.6/site-packages/google/cloud/spanner/streamed.py", line166, in__iter__self.consume_next() # raises StopIterationFile"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/.nox/sys-3-6/lib/python3.6/site-packages/google/cloud/spanner/streamed.py", line132, inconsume_nextresponse=six.next(self._response_iterator)
File"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/.nox/sys-3-6/lib/python3.6/site-packages/grpc/_channel.py", line363, in__next__returnself._next()
File"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/.nox/sys-3-6/lib/python3.6/site-packages/grpc/_channel.py", line357, in_nextraiseselfgrpc._channel._Rendezvous: <_RendezvousofRPCthatterminatedwith (StatusCode.INVALID_ARGUMENT, Transactionwasstartedinadifferentsession.)>______________________TestSessionAPI.test_read_w_ranges_______________________Traceback (mostrecentcalllast):
File"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/tests/system/test_system.py", line846, intest_read_w_rangesself.TABLE, self.COLUMNS, keyset))
File"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/.nox/sys-3-6/lib/python3.6/site-packages/google/cloud/spanner/streamed.py", line166, in__iter__self.consume_next() # raises StopIterationFile"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/.nox/sys-3-6/lib/python3.6/site-packages/google/cloud/spanner/streamed.py", line132, inconsume_nextresponse=six.next(self._response_iterator)
File"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/.nox/sys-3-6/lib/python3.6/site-packages/grpc/_channel.py", line363, in__next__returnself._next()
File"/home/tseaver/projects/agendaless/Google/src/google-cloud-python/spanner/.nox/sys-3-6/lib/python3.6/site-packages/grpc/_channel.py", line357, in_nextraiseselfgrpc._channel._Rendezvous: <_RendezvousofRPCthatterminatedwith (StatusCode.INVALID_ARGUMENT, Transactionwasstartedinadifferentsession.)>

I tried putting an explicit snapshot.begin() before the repeated read / execute_sql calls, but that didnt' change anything. Would the back-end be somehow be passing back a new transaction ID for the second read / execute_sql request (which does succeed)?

tseaver · 2017-07-24T19:32:03Z

Looks like the back-end passes back an empty transaction ID for read requests after the first one. 0966806 keeps us from clearing it in that case.

vkedia · 2017-07-24T19:56:05Z

Thats right. Transaction field is only set if the read or query started a new transaction.
https://cloud.google.com/spanner/docs/reference/rpc/google.spanner.v1#google.spanner.v1.ResultSetMetadata

tseaver · 2017-07-24T20:52:22Z

The new system tests are passing.

dhermes · 2017-07-24T20:58:39Z

@tseaver What are you looking for here in terms of review?

tseaver · 2017-07-24T21:18:38Z

@dhermes the commits which are new since @lukesneeringer gave an LGTM on Thursday (the rest are just rebase to fix conflicts). a5219a5 is the hash of that rebased commit, so the diff would be: a5219a5...spanner-multi_use_snapshot

tseaver · 2017-07-26T19:50:07Z

@vkedia, @dhermes any issues remaining?

dhermes · 2017-07-26T20:38:41Z

LGTM

bjwatson · 2017-07-26T23:04:30Z

@tseaver I just saw this. I think that @vkedia will want to review this more when he returns from vacation on Monday. I guess if he finds anything else, it can be addressed in a separate PR.

Let's remain Alpha until next week. (FYI @lukesneeringer)

bjwatson · 2017-08-08T00:34:15Z

@vkedia Do you plan to finish reviewing this post-merge?

tseaver added api: spanner Issues related to the Spanner API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Jul 17, 2017

tseaver requested review from dhermes and lukesneeringer July 17, 2017 18:38

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 17, 2017

dhermes reviewed Jul 19, 2017
View reviewed changes

tseaver force-pushed the spanner-multi_use_snapshot branch 2 times, most recently from ad62692 to d445a73Compare July 19, 2017 22:43

dhermes reviewed Jul 19, 2017
View reviewed changes

Comment threadspanner/tests/system/test_system.py Outdated

def test_multiuse_snapshot_read_isolation_exact_staleness(self):
import time
from datetime import timedelta
This comment was marked as spam.
Sign in to view

vkedia reviewed Jul 19, 2017
View reviewed changes

Comment threadspanner/google/cloud/spanner/snapshot.py
return StreamedResultSet(iterator)

def execute_sql(self, sql, params=None, param_types=None, query_mode=None,
resume_token=b''):
This comment was marked as spam.
Sign in to view
This comment was marked as spam.
Sign in to view

vkedia reviewed Jul 19, 2017
View reviewed changes

Comment threadspanner/google/cloud/spanner/snapshot.py Outdated
``exact_staleness`` old.

:type multi_use: :class:`bool`
:param multi_use: If true, the first read operation creates a read-only
This comment was marked as spam.
Sign in to view

vkedia reviewed Jul 19, 2017
View reviewed changes

Comment threadspanner/google/cloud/spanner/snapshot.py
if self._multi_use:
return StreamedResultSet(iterator, source=self)
else:
return StreamedResultSet(iterator)
This comment was marked as spam.
Sign in to view

lukesneeringer approved these changes Jul 20, 2017
View reviewed changes

tseaver added 8 commits July 24, 2017 13:12

Add 'multi_use' param to 'Snapshot'.
cdfa43a
- Convert 'Database.snapshot' and 'Session.snapshot' factories to take / forward '**kw'.

Pass txn options as 'begin' when 'multi_use' is set.
43f44e4

Add 'source' parameter to 'StreamedResultIterator'.
5d3964f
- When reading / executing SQL for a multi-use snapshot, pass the snapshot as the iterator's source.

Use real protobuf classes:
2e5686f
- PartialResultSet - ResultSetMetadata - ResultSetStats.

On first read, propagate transaction id to source.
bd15294
- Source will only be set for multi-use snapshots.

For subsequent reads from multi-use snapshots, pass the transaction ID.
9f07550

Add system tests exercising multi-use snapshots.
22ad580

Add systets for multiuse snapshots w/ read_timestamp / exact_staleness.
3fc3aef

tseaver force-pushed the spanner-multi_use_snapshot branch from 799ce25 to 230d715Compare July 24, 2017 17:49

Address docstring review comment.
1e72890

Add guard against reusing single-use snapshots.
ec38b8b

tseaver mentioned this pull request Jul 24, 2017
Expose read-isolation parameters in 'Database.read'/'Database.execute_sql' #3659
Closed

tseaver added 2 commits July 24, 2017 15:04

Add guard against pending transaction for multi-use snapshots.
8244af9

Update system tests for proper multi-use snapshot semantics.
5dc09f8

tseaver force-pushed the spanner-multi_use_snapshot branch from 1fff028 to 5dc09f8Compare July 24, 2017 19:15

Avoid clearning snapshot txn ID when back-end returns blank.
0966806

Untangle 'Transaction._id' vs. '_SnapshotBase._transaction_id'.
10f7126

tseaver mentioned this pull request Jul 24, 2017
Add a test which provokes abort-during-read during 'run_in_transaction'. #3663
Merged

tseaver merged commit e273319 into masterJul 26, 2017

tseaver deleted the spanner-multi_use_snapshot branch July 26, 2017 22:00

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 21, 2017
Implement multi-use snapshots (googleapis#3615)
d9a3dbe

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
Implement multi-use snapshots (googleapis#3615)
e2ec1e1

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
Implement multi-use snapshots (googleapis#3615)
5b82533

parthea pushed a commit that referenced this pull request Mar 9, 2026
Implement multi-use snapshots (#3615)
d724687

Uh oh!

Conversation

tseaver commented Jul 17, 2017

Uh oh!

dhermes left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

dhermes left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

tseaver commented Jul 19, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

vkedia commented Jul 20, 2017

Uh oh!

lukesneeringer left a comment

Choose a reason for hiding this comment

Uh oh!

vkedia commented Jul 20, 2017

Uh oh!

lukesneeringer commented Jul 20, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkedia commented Jul 21, 2017

Uh oh!

tseaver commented Jul 24, 2017

Uh oh!

tseaver commented Jul 24, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkedia commented Jul 24, 2017

lukesneeringer commented Jul 20, 2017•
edited
Loading

tseaver commented Jul 24, 2017•
edited
Loading

tseaver commented Jul 24, 2017•
edited
Loading