Add Spanner stale read sample. #475

jmdobry · 2017-09-06T15:05:21Z

No description provided.

codecov · 2017-09-06T15:13:25Z

Codecov Report

Merging #475 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #475   +/-   ##
=======================================
  Coverage   97.16%   97.16%           
=======================================
  Files          13       13           
  Lines         458      458           
=======================================
  Hits          445      445           
  Misses         13       13

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fca8cf...e5dabb4. Read the comment docs.

ace-n · 2017-09-06T20:06:32Z

spanner/crud.js

+  const instance = spanner.instance(instanceId);
+  const database = instance.database(databaseId);
+
+  // Read rows from the Albums table


Nit: inconsistent comment tense.

ace-n · 2017-09-06T20:07:09Z

spanner/crud.js

+  };
+
+  const options = {
+    // Guarantees that all writes that have committed more than 10 seconds ago


Nit: remove that have and combine into one line

ace-n · 2017-09-06T20:08:20Z

spanner/crud.js

+
+      rows.forEach((row) => {
+        const json = row.toJSON();
+        console.log(`SingerId: ${json.SingerId.value}, AlbumId: ${json.AlbumId.value}, AlbumTitle: ${json.AlbumTitle}, MarketingBudget: ${json.MarketingBudget ? json.MarketingBudget.value : ''}`);


Can we split these into separate lines? This is a lot to read at once...

ace-n · 2017-09-06T20:09:06Z

spanner/crud.js

@@ -189,10 +233,17 @@ const cli = require(`yargs`)
    {},
    (opts) => readData(opts.instanceName, opts.databaseName)
  )
+  .command(
+    `read-stale <instanceName> <databaseName>`,
+    `Reads data in an example Cloud Spanner table.`,


Nit: mention/explain stale reads in the command description?

e.g. Reads _stale_ data in an example Cloud Spanner table.

ace-n · 2017-09-06T20:15:41Z

spanner/system-test/spanner.test.js

-  const output = await tools.runAsync(`${schemaCmd} createDatabase "${INSTANCE_ID}" "${DATABASE_ID}"`, cwd);
-  t.true(output.includes(`Waiting for operation on ${DATABASE_ID} to complete...`));
-  t.true(output.includes(`Created database ${DATABASE_ID} on instance ${INSTANCE_ID}.`));
+  const results = await tools.runAsyncWithIO(`${schemaCmd} createDatabase "${INSTANCE_ID}" "${DATABASE_ID}"`, cwd);


What's the point of this? (Having stderr in the test printout?)

stderr should be ignored unless we expect an error to occur. (If an error occurs when it shouldn't, our test should fail - though perhaps not silently.)

I've discover lately that sometimes tests/commands fail silently (return a 0 exit code), but do print some useful info to stderr. This improves test debuggability.

ace-n · 2017-09-06T20:17:03Z

spanner/system-test/spanner.test.js

-  t.true(output.includes(`Inserted data.`));
+  const results = await tools.runAsyncWithIO(`${crudCmd} insert ${INSTANCE_ID} ${DATABASE_ID}`, cwd);
+  const output = results.stdout + results.stderr;
+  t.regex(output, new RegExp(`Inserted data.`));


Replace t.regex(output, new RegExp($STRING)) with t.regex(output, /$STRING/). Apply this comment throughout your PR.

ace-n · 2017-09-06T20:18:30Z

spanner/system-test/spanner.test.js

+  // read-stale-data reads data that is exactly 10 seconds old.  So, make sure
+  // 10 seconds have elapsed since the update_data test.
+  return new Promise((resolve, reject) => {
+    setTimeout(async () => {


Nit: can you promisify the setTimeout() call, and wrap the tests in a then as was done here?

jmdobry · 2017-09-14T21:18:37Z

Comments addressed, merging.

🤖 I have created a release \*beep\* \*boop\* --- ### [2.4.3](https://www.github.com/googleapis/nodejs-kms/compare/v2.4.2...v2.4.3) (2021-07-12) ### Bug Fixes * **deps:** google-gax v2.17.1 ([#474](https://www.github.com/googleapis/nodejs-kms/issues/474)) ([1e1a3fc](https://www.github.com/googleapis/nodejs-kms/commit/1e1a3fcb437cdfb86e3fcb06ac34a329898b970a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release \*beep\* \*boop\* --- ## [2.4.0](https://www.github.com/googleapis/nodejs-dataproc/compare/v2.3.0...v2.4.0) (2021-04-26) ### Features * a bundle of new features for Dataproc V1 ([#475](https://www.github.com/googleapis/nodejs-dataproc/issues/475)) ([4803be4](https://www.github.com/googleapis/nodejs-dataproc/commit/4803be4f4b63833072aa50e29e8b081c363f6dde)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

jmdobry self-assigned this Sep 6, 2017

jmdobry requested a review from ace-n September 6, 2017 15:05

Add Spanner stale read sample.

d5abeb3

jmdobry force-pushed the spanner-stale-read branch from 57fed2e to d5abeb3 Compare September 6, 2017 15:46

ace-n suggested changes Sep 6, 2017

View reviewed changes

Address comments.

e5dabb4

jmdobry merged commit 79dc773 into master Sep 14, 2017

jmdobry deleted the spanner-stale-read branch September 14, 2017 21:30

grayside pushed a commit that referenced this pull request Oct 26, 2022

docs(samples): update samples to latest standards (#475)

acb9c06

grayside mentioned this pull request Oct 26, 2022

migrate code from googleapis/nodejs-tasks #2792

Merged

grayside pushed a commit that referenced this pull request Nov 3, 2022

docs(samples): update samples to latest standards (#475)

b148833

NimJay mentioned this pull request Nov 11, 2022

migrate code from googleapis/nodejs-kms #2839

Merged

Shabirmean pushed a commit that referenced this pull request Feb 16, 2023

fix(deps): update dependency @google-cloud/pubsub to v2 (#475)

48d55f3

Shabirmean mentioned this pull request Feb 16, 2023

samples: migrate code from googleapis/nodejs-dlp #3035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Spanner stale read sample. #475

Add Spanner stale read sample. #475

jmdobry commented Sep 6, 2017

codecov bot commented Sep 6, 2017 •

edited

Loading

ace-n Sep 6, 2017

jmdobry Sep 12, 2017

ace-n Sep 6, 2017

jmdobry Sep 12, 2017

ace-n Sep 6, 2017

jmdobry Sep 12, 2017

ace-n Sep 6, 2017

jmdobry Sep 12, 2017

ace-n Sep 6, 2017 •

edited

Loading

jmdobry Sep 12, 2017

ace-n Sep 6, 2017

jmdobry Sep 12, 2017

ace-n Sep 6, 2017

jmdobry Sep 12, 2017

jmdobry commented Sep 14, 2017

Add Spanner stale read sample. #475

Add Spanner stale read sample. #475

Conversation

jmdobry commented Sep 6, 2017

codecov bot commented Sep 6, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ace-n Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmdobry commented Sep 14, 2017

codecov bot commented Sep 6, 2017 •

edited

Loading

ace-n Sep 6, 2017 •

edited

Loading