common/spanner: introduce `debug` module and nested transaction warning by stephenplusplus · Pull Request #2517 · googleapis/google-cloud-node

stephenplusplus · 2017-08-09T16:50:38Z

callmehiphop

Would it be worth the trouble to integrate with a user-specified logger? I'm mostly curious as to whether or not we would want to integrate with google-cloud-logging (I'm not very familiar with it, so it might not even be possible)

Sign in to view


    describe('read only', function() {
-      it('should run a read only transaction', function(done) {
+      it.only('should run a read only transaction', function(done) {


bjwatson

@vkedia Are these changes sufficient for Node, given the complexities of enforcing this more strongly?

I also believe that the docs will be updated to make it clear that nested transactions aren't supported (if this hasn't been done already).

Sign in to view

  options = extend({}, options);

+  this.debug([
+    'Stating a transaction. Note that nested transactions are not currently',


stephenplusplus · 2017-09-10T19:16:27Z

bump

Sign in to view

  options = extend({}, options);

+  this.debug([
+    'Starting a transaction. Note that nested transactions are not currently',


vkedia · 2017-09-11T19:05:36Z

Looking at this code, it is not clear to me exactly when would this warning be triggered?

stephenplusplus · 2017-09-12T13:38:56Z

The user opts in to this warning behavior by using the popular debug module. When they start their app, it would look like:

export DEBUG=@google-cloud/spanner; npm start

The line itself will be printed to their terminal any time a transaction is started.

vkedia · 2017-09-12T16:43:50Z

Oh so this would be printed every time a transaction is started and not just if we detect nested transactions?

stephenplusplus · 2017-09-12T17:01:24Z

Right, we couldn't come up with a semi-reliable way to detect nested transactions without an API redesign. By always warning, in the worst case, it's excess noise to the user. But, by using the debug module, the user is expecting to receive a steady stream of output, so I wouldn't expect this to stand out in a way that's overly obtrusive.

vkedia · 2017-09-19T18:46:20Z

@stephenplusplus I am not sure how much utility this would have on top of documentation. It just seems strange that we are telling them something which they might even know exist. It might infact confuse users and they might worry why they are getting this warning and maybe they are doing something wrong.
I would say let us not merge this and instead just document this (if not already documented).

googlebot · 2017-09-21T14:37:06Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

stephenplusplus · 2017-09-21T14:39:28Z

@vkedia updated the docs to make the same mention. Feel free to fork this PR if you have a better idea for the implementation, or words to use to explain.

googlebot · 2017-09-21T14:39:32Z

CLAs look good, thanks!

coveralls · 2017-09-21T14:45:42Z

Coverage remained the same at 100.0% when pulling b900f6c on stephenplusplus:spp--2361 into badd1b8 on GoogleCloudPlatform:master.

Sign in to view

 * atomically at a single logical point in time across columns, rows, and tables
 * in a database.
 *
+ * Note that Cloud Spanner does not support nested transactions.


coveralls · 2017-10-10T21:39:52Z

Coverage remained the same at 100.0% when pulling 5fb7905 on stephenplusplus:spp--2361 into badd1b8 on GoogleCloudPlatform:master.

Without this, it's difficult to create an informational/delegated retry function, that wraps the default and logs the outcome / does some additional logic

stephenplusplus added api: spanner Issues related to the Spanner API. core labels Aug 9, 2017

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

stephenplusplus added status: do not merge and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 9, 2017

stephenplusplus requested a review from callmehiphop August 9, 2017 16:50

stephenplusplus mentioned this pull request Aug 9, 2017

Cloud Spanner Client should prevent nested transactions #2361

Closed

callmehiphop reviewed Aug 9, 2017

View reviewed changes

Comment thread packages/spanner/system-test/spanner.js Outdated

describe('read only', function() {

it('should run a read only transaction', function(done) {

it.only('should run a read only transaction', function(done) {

This comment was marked as spam.

Sign in to view

bjwatson reviewed Aug 9, 2017

View reviewed changes

Comment thread packages/spanner/src/database.js Outdated

options = extend({}, options);

this.debug([

'Stating a transaction. Note that nested transactions are not currently',

This comment was marked as spam.

Sign in to view

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

stephenplusplus mentioned this pull request Aug 21, 2017

pubsub throws 'Service Unavailable' GoogleCloudPlatform/nodejs-docs-samples#277

Closed

vkedia reviewed Sep 11, 2017

View reviewed changes

Comment thread packages/spanner/src/database.js Outdated

options = extend({}, options);

this.debug([

'Starting a transaction. Note that nested transactions are not currently',

This comment was marked as spam.

Sign in to view

stephenplusplus and others added 3 commits September 21, 2017 10:35

common: expose debug module

8063199

spanner: use debug to communicate nested transaction warning

1663b09

just put it in the docs

f526847

stephenplusplus force-pushed the spp--2361 branch from fe87bc5 to f526847 Compare September 21, 2017 14:37

stephenplusplus requested a review from lukesneeringer as a code owner September 21, 2017 14:37

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Sep 21, 2017

rm .only

b900f6c

stephenplusplus added docs and removed status: do not merge cla: no This human has *not* signed the Contributor License Agreement. labels Sep 21, 2017

stephenplusplus removed the core label Sep 21, 2017

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

vkedia reviewed Sep 21, 2017

View reviewed changes

Comment thread packages/spanner/src/database.js Outdated

* atomically at a single logical point in time across columns, rows, and tables

* in a database.

*

* Note that Cloud Spanner does not support nested transactions.

This comment was marked as spam.

Sign in to view

Update database.js

5fb7905

stephenplusplus merged commit 623d302 into googleapis:master Oct 10, 2017

renovate Bot mentioned this pull request Feb 20, 2018

Update dependency @google-cloud/error-reporting to v0.2.3 - autoclosed nLight/objektiv#18

Closed

sofisl pushed a commit that referenced this pull request Feb 25, 2026

fix(retry): export RETRYABLE_ERR_FN_DEFAULT (#2517)

5edccca

Without this, it's difficult to create an informational/delegated retry function, that wraps the default and logs the outcome / does some additional logic

sofisl pushed a commit that referenced this pull request Feb 26, 2026

common/spanner: add nested transaction warning (#2517)

1800ed7

Conversation

stephenplusplus commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

callmehiphop left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

bjwatson left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

stephenplusplus commented Sep 10, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

vkedia commented Sep 11, 2017

Uh oh!

stephenplusplus commented Sep 12, 2017

Uh oh!

vkedia commented Sep 12, 2017

Uh oh!

stephenplusplus commented Sep 12, 2017

Uh oh!

vkedia commented Sep 19, 2017

Uh oh!

googlebot commented Sep 21, 2017

Uh oh!

stephenplusplus commented Sep 21, 2017

Uh oh!

googlebot commented Sep 21, 2017

Uh oh!

coveralls commented Sep 21, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

coveralls commented Oct 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stephenplusplus commented Aug 9, 2017 •

edited

Loading