common/spanner: introduce `debug` module and nested transaction warning #2517

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)

packages/spanner/system-test/spanner.js

@@ -2702,7 +2702,7 @@ var spanner = new Spanner(env);
    });

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

packages/spanner/src/database.js

@@ -892,6 +892,11 @@ Database.prototype.runTransaction = function(options, runFn) {

  options = extend({}, options);

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


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

bump

packages/spanner/src/database.js

@@ -892,6 +892,11 @@ Database.prototype.runTransaction = function(options, runFn) {

  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.

packages/spanner/src/database.js

@@ -787,6 +787,8 @@ Database.prototype.runStream = function(query, options) {
 * 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.

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

bjwatson reviewed Aug 9, 2017

View reviewed changes

packages/spanner/src/database.js Outdated

@@ -892,6 +892,11 @@ Database.prototype.runTransaction = function(options, runFn) {

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

packages/spanner/src/database.js Outdated

@@ -892,6 +892,11 @@ Database.prototype.runTransaction = function(options, runFn) {

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common/spanner: introduce `debug` module and nested transaction warning #2517

common/spanner: introduce `debug` module and nested transaction warning #2517

stephenplusplus commented Aug 9, 2017 •

edited

Loading

callmehiphop left a comment

This comment was marked as spam.

bjwatson left a comment

This comment was marked as spam.

stephenplusplus commented Sep 10, 2017

This comment was marked as spam.

vkedia commented Sep 11, 2017

stephenplusplus commented Sep 12, 2017

vkedia commented Sep 12, 2017

stephenplusplus commented Sep 12, 2017

vkedia commented Sep 19, 2017

googlebot commented Sep 21, 2017

stephenplusplus commented Sep 21, 2017

googlebot commented Sep 21, 2017

coveralls commented Sep 21, 2017

This comment was marked as spam.

coveralls commented Oct 10, 2017

common/spanner: introduce debug module and nested transaction warning #2517

common/spanner: introduce debug module and nested transaction warning #2517

Conversation

stephenplusplus commented Aug 9, 2017 • edited Loading

callmehiphop left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

bjwatson left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

stephenplusplus commented Sep 10, 2017

This comment was marked as spam.

vkedia commented Sep 11, 2017

stephenplusplus commented Sep 12, 2017

vkedia commented Sep 12, 2017

stephenplusplus commented Sep 12, 2017

vkedia commented Sep 19, 2017

googlebot commented Sep 21, 2017

stephenplusplus commented Sep 21, 2017

googlebot commented Sep 21, 2017

coveralls commented Sep 21, 2017

This comment was marked as spam.

coveralls commented Oct 10, 2017

common/spanner: introduce `debug` module and nested transaction warning #2517

common/spanner: introduce `debug` module and nested transaction warning #2517

stephenplusplus commented Aug 9, 2017 •

edited

Loading