feat: inline BeginTransaction with first statement by ko3a4ok · Pull Request #1692 · googleapis/nodejs-spanner

ko3a4ok · 2022-08-06T02:31:25Z

Add inline BeginTransaction for the first read or executeSql request in a transaction.

Each transaction doesn't call beginTransanction before its start. Instead, all creation transaction parameters are passed within the first read or executeSql request.
In case of a transaction restart, beginTransaction is called explicitly.

olavloite · 2022-08-07T11:26:30Z

src/transaction.ts

    if (this.id) {
      transaction.id = this.id as Uint8Array;
+    } else if (typeof this._options.readWrite !== 'undefined') {
+      transaction.begin = this._options;


Would you mind adding a test that verifies the following:

A query is the first statement in a read/write transaction. A transaction ID is successfully returned by initial request.

One or more PartialResultSets are returned by the stream, with (at least) one of them returning a resume token.

The stream fails halfway with an UNAVAILABLE error and the stream is restarted with a resume token.

Step 3 should use the transaction ID that was returned by step 1, and not include a BeginTransaction option.

(https://github.com/googleapis/nodejs-spanner/pull/1253/files is a very old implementation for the same. Some of the tests there might be re-usable.)

Added. thanks a lot for this comment: it helped to find a bug in the code

olavloite · 2022-08-09T12:56:08Z

src/transaction.ts

@@ -1034,6 +1029,8 @@ export class Snapshot extends EventEmitter {
      const transaction: spannerClient.spanner.v1.ITransactionSelector = {};
      if (this.id) {


This does not take into account that a transaction could in theory have multiple requests in flight at the same time. If a transaction starts out with sending two SELECT statements to the backend, it might very well be that the first that is sent has not yet returned a transaction id before the second is being sent. That will cause both requests to include a BeginTransaction option.

Consider the following test case (the getRowCountFromStreamingSql function is defined in test/Spanner.ts):

it('should only inline one begin transaction', async () => { const database = newTestDatabase(); await database.runTransactionAsync(async tx => { const rowCount1 = getRowCountFromStreamingSql(tx, {sql: selectSql}); const rowCount2 = getRowCountFromStreamingSql(tx, {sql: selectSql}); await Promise.all([rowCount1, rowCount2]); await tx.commit(); }); await database.close(); let request = spannerMock.getRequests().find(val => { return (val as v1.ExecuteSqlRequest).sql; }) as v1.ExecuteSqlRequest; assert.ok(request, 'no ExecuteSqlRequest found'); assert.deepStrictEqual(request.transaction!.begin!.readWrite, {}); assert.strictEqual(request.sql, selectSql); request = spannerMock .getRequests() .slice() .reverse() .find(val => { return (val as v1.ExecuteSqlRequest).sql; }) as v1.ExecuteSqlRequest; assert.ok(request, 'no ExecuteSqlRequest found'); assert.strictEqual(request.sql, selectSql); assert.ok(request.transaction!.id, 'TransactionID is not set.'); const beginTxnRequest = spannerMock.getRequests().find(val => { return (val as v1.BeginTransactionRequest).options?.readWrite; }) as v1.BeginTransactionRequest; assert.ok(!beginTxnRequest, 'beginTransaction was called'); });

You're absolutely right. Added a lock that prevents a multiple inline begin transaction at the same time. Thanks!

…on happens; make sure stream requests uses a transaction id from the first response even if the stream fails halfway with an UNAVAILABLE error

surbhigarg92 · 2022-08-22T06:29:09Z

src/transaction.ts

      json,
      jsonOptions,
      maxResumeRetries,
+    }).on('response', response => {


What would happen if the call fails?
As per the design we should make an explicit begin transaction call if the call fails.

cc: @olavloite

if the call fails, the transaction is created explicitly with "BeginTransaction" rpc call.
It's covered in the test: "should use beginTransaction on retry".

Is this done at the backend or do we explicitly call begin transaction from the client side?

from the client side

asthamohta · 2022-08-23T05:22:47Z

src/transaction.ts


    if (this.id) {
      transaction.id = this.id as Uint8Array;
+    } else if (typeof this._options.readWrite !== 'undefined') {


Should this rather be else if(this._options.readWrite)

yes, fixed, thanks

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

surbhigarg92 · 2022-09-14T09:49:13Z

@ko3a4ok Can you please fix the failing test cases ?

surbhigarg92 · 2022-09-14T09:50:46Z

@ko3a4ok We should also remove the transaction.begin call at line and add the appropriate test cases for the same.

ko3a4ok · 2022-09-14T23:15:43Z

@ko3a4ok We should also remove the transaction.begin call at line and add the appropriate test cases for the same.

@surbhigarg92, unfortunately, if we just remove it, the explicit beginTransaction call won't be called on the retry. I updated to call it only after the first attempt. Please let me know if it makes sense, or is it better to move the call transaction.begin() somewhere here.

surbhigarg92 · 2022-09-19T11:06:57Z

@ko3a4ok We should also remove the transaction.begin call at line and add the appropriate test cases for the same.

@surbhigarg92, unfortunately, if we just remove it, the explicit beginTransaction call won't be called on the retry. I updated to call it only after the first attempt. Please let me know if it makes sense, or is it better to move the call transaction.begin() somewhere here.

This looks fine. Only thing I am wondering is that with this implementation, we are calling transaction.begin() in all possible failure cases.
@olavloite Do you think there will be any particular error case where we wan't to skip calling transaction.begin()

ko3a4ok · 2022-09-20T00:58:01Z

@ko3a4ok I believe we are missing 2 use cases for Inline BeginTransaction implementation.

Mutation - Consider a transaction which contains only mutation operations. In that case, first rpc in the transaction will be commit transaction. To handle this scenario , we would need to make an explicit begin transaction rpc.

ExecuteBatchDML - Transaction needs to be inlined for ExecuteBatchDML operation as well. Special handling is required for this use case.

ExecuteBatchDml is also slightly odd since either the call itself can fail, or it can return success with a non-OK Status in the ExecuteBatchDmlResponse. In the latter case, there may still be one or more ResultSets returned, with the first containing a transaction ID in its ResultSetMetadata. The upshot is that the client should check ExecuteBatchDmlResponse::result_set for a transaction prior to considering ExecuteBatchDmlResponse::status.

Regarding 1.: in this case, we should not create any transaction at all because CommitRequest will include singleUseTransaction parameter, and transactionId will be empty. This is how it works now, and inlined begin transaction shouldn't affect this implementation.

Regarding 2.: I completely forgot about that, thanks for bringing this up. Added in 2caaa0f

gcf-merge-on-green · 2022-11-02T12:52:13Z

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

…egin-txns

feat: add request/response debug logging to gapics, update templates to gax 5 and node 18 (#1671) fix: add json files to tsconfig templates (#1692) (ba6be1d) PiperOrigin-RevId: 735896588 Source-Link: googleapis/googleapis@3419af7 Source-Link: googleapis/googleapis-gen@f35ba11 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZjM1YmExMTQyZjRlMTY4MjIyMzI3ZDg5MmI1ZjZlZTkwOGU1ZDQ2MSJ9

* feat: add a last field in the PartialResultSet docs: A comment for field `rows` in message `.google.spanner.v1.ResultSet` is changed docs: A comment for field `stats` in message `.google.spanner.v1.ResultSet` is changed docs: A comment for field `precommit_token` in message `.google.spanner.v1.ResultSet` is changed docs: A comment for field `values` in message `.google.spanner.v1.PartialResultSet` is changed docs: A comment for field `chunked_value` in message `.google.spanner.v1.PartialResultSet` is changed docs: A comment for field `stats` in message `.google.spanner.v1.PartialResultSet` is changed docs: A comment for field `precommit_token` in message `.google.spanner.v1.PartialResultSet` is changed docs: A comment for message `ResultSetMetadata` is changed docs: A comment for field `row_type` in message `.google.spanner.v1.ResultSetMetadata` is changed docs: A comment for message `ResultSetStats` is changed docs: A comment for field `query_plan` in message `.google.spanner.v1.ResultSetStats` is changed docs: A comment for field `row_count_lower_bound` in message `.google.spanner.v1.ResultSetStats` is changed PiperOrigin-RevId: 730849734 Source-Link: googleapis/googleapis@fe0fa26 Source-Link: googleapis/googleapis-gen@16051b5 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMTYwNTFiNTkxN2I3NWY2MDNjY2I1ZjQ3N2UyYTQ2NDdiYTExZmE4MiJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * chore: update copyright year for auto-generated protos PiperOrigin-RevId: 731693666 Source-Link: googleapis/googleapis@fd510f8 Source-Link: googleapis/googleapis-gen@42097f7 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiNDIwOTdmN2NjNzZlZTdiZGZlY2FlYWVhMTE1YWYyMzU3MTZmZGRiMSJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * chore: update copyright year for auto-generated protos PiperOrigin-RevId: 732130682 Source-Link: googleapis/googleapis@9415ba0 Source-Link: googleapis/googleapis-gen@2905f83 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMjkwNWY4MzM3NTZjMmIyMGIzMjgyYmU4NGI1MTFlMDQwZmU1NGYzMyJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * chore: update typescript gapic generator to 4.8.1 feat: add request/response debug logging to gapics, update templates to gax 5 and node 18 (#1671) fix: add json files to tsconfig templates (#1692) (ba6be1d) PiperOrigin-RevId: 735896588 Source-Link: googleapis/googleapis@3419af7 Source-Link: googleapis/googleapis-gen@f35ba11 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZjM1YmExMTQyZjRlMTY4MjIyMzI3ZDg5MmI1ZjZlZTkwOGU1ZDQ2MSJ9 * feat: await/catch promises, and update listOperationsAsync return type PiperOrigin-RevId: 738212310 Source-Link: googleapis/googleapis@803b234 Source-Link: googleapis/googleapis-gen@4f44bd2 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiNGY0NGJkMmJhYThkZDVhNzFjYTBjZWJkYjE2NGMzYzM0MzQxZWQ4NyJ9 * chore: run post-processor locally * chore: migrate to node 18 * fix: imports * changes in tsconfig * fix: import errors * remove script * update tsconfig * fix: eslint errors * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * downgrade dependencies arrify, pqueue, time-span, stack-trace * add spanProcessors property and remove addSpanProcessor * fix: eslint issues and unit tests * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * fix: unit test * fix: presubmit errors * add node-18 directory * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * rename node-18 directory * update execa import * downgrade execa and p-limit package to support the tests run against emulator * downgrade uuid version * update system-test-against-emulator.yaml * add system-test-multiplexed-session.cfg in node18 * update image and digest in owlbot yaml * downgrade execa and p-limit * fix: presubmit error for tests * update coverage to node 18 version * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * update samples-test.sh * update sha and workflows * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * presubmit fix * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * refactor transaction and session-pool src --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Sofia Leon <sofialeon@google.com>

feat: add request/response debug logging to gapics, update templates to gax 5 and node 18 (#1671) fix: add json files to tsconfig templates (#1692) (ba6be1d) PiperOrigin-RevId: 735896588 Source-Link: googleapis/googleapis@3419af7 Source-Link: googleapis/googleapis-gen@f35ba11 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZjM1YmExMTQyZjRlMTY4MjIyMzI3ZDg5MmI1ZjZlZTkwOGU1ZDQ2MSJ9

feat: inline BeginTransaction with first statement

3d06c45

ko3a4ok requested review from a team August 6, 2022 02:31

product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/nodejs-spanner API. labels Aug 6, 2022

olavloite reviewed Aug 9, 2022

View reviewed changes

asthamohta requested a review from surbhigarg92 August 11, 2022 12:13

asthamohta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 11, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 11, 2022

fix: prevent multiple begin transactions happen during a race conditi…

8d5c6d6

…on happens; make sure stream requests uses a transaction id from the first response even if the stream fails halfway with an UNAVAILABLE error

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Aug 12, 2022

surbhigarg92 reviewed Aug 22, 2022

View reviewed changes

asthamohta reviewed Aug 23, 2022

View reviewed changes

ko3a4ok and others added 2 commits August 23, 2022 21:37

Merge branch 'googleapis:main' into inline-begin-txns

15944f5

fix: minor

dd9891e

surbhigarg92 added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Sep 14, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 14, 2022

🦉 Updates from OwlBot post-processor

6b57710

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 14, 2022

ko3a4ok and others added 2 commits September 14, 2022 13:49

Merge branch 'googleapis:main' into inline-begin-txns

976ee59

fix: call explicit begin transaction only after the first attempt.

154872b

feat: add inline begin transaction for batch DML requests

2caaa0f

Merge branch 'main' into inline-begin-txns

9929be5

surbhigarg92 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 2, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 2, 2022

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 2, 2022

surbhigarg92 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 3, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 3, 2022

ko3a4ok added 2 commits November 3, 2022 17:49

fix: samples test

2f226e1

Merge remote-tracking branch 'origin/inline-begin-txns' into inline-b…

6d94a86

…egin-txns

surbhigarg92 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 4, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 4, 2022

Merge branch 'main' into inline-begin-txns

b0bccf5

surbhigarg92 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 4, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 4, 2022

fix: system test

346abea

surbhigarg92 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 7, 2022

fix: system emulator test

311a80f

yoshi-kokoro removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Nov 7, 2022

surbhigarg92 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 7, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 7, 2022

surbhigarg92 added the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 7, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Nov 7, 2022

surbhigarg92 merged commit d1b95d2 into googleapis:main Nov 7, 2022

release-please bot mentioned this pull request Nov 7, 2022

chore(main): release 6.5.0 #1734

Merged

mrichardEA mentioned this pull request Aug 23, 2023

Transaction Promise.all with multiple queries results in Error: stream.push() after EOF #1908

Closed

gcf-owl-bot bot mentioned this pull request Mar 12, 2025

feat: await/catch promises, and update listOperationsAsync return type #2240

Closed

1 task

		@@ -1034,6 +1029,8 @@ export class Snapshot extends EventEmitter {
		const transaction: spannerClient.spanner.v1.ITransactionSelector = {};
		if (this.id) {

Conversation

ko3a4ok commented Aug 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surbhigarg92 Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surbhigarg92 commented Sep 14, 2022

Uh oh!

surbhigarg92 commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ko3a4ok commented Sep 14, 2022

Uh oh!

surbhigarg92 commented Sep 19, 2022

Uh oh!

ko3a4ok commented Sep 20, 2022

Uh oh!

gcf-merge-on-green bot commented Nov 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

surbhigarg92 Aug 22, 2022 •

edited

Loading

surbhigarg92 commented Sep 14, 2022 •

edited

Loading