Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: sample on setting per-request gaxOptions #1210

Merged

Conversation

laljikanjareeya
Copy link
Contributor

Fixes #1203

@laljikanjareeya laljikanjareeya requested review from a team as code owners August 7, 2020 04:19
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 7, 2020
samples/dml.js Outdated Show resolved Hide resolved
Copy link
Contributor

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, 👍 . Only need a small change.

@hengfengli hengfengli added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Aug 7, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 7, 2020
samples/dml.js Outdated
@@ -630,6 +630,74 @@ async function updateUsingBatchDml(instanceId, databaseId, projectId) {
// [END spanner_dml_batch_update]
}

async function executeSqlWithCustomTimeoutAndRetrySettings(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the Samples Test fail due to the async prefix here? I don't see what the difference is, comparing to

function insertUsingDml(instanceId, databaseId, projectId) {
// [START spanner_dml_standard_insert]
// Imports the Google Cloud client library
const {Spanner} = require('@google-cloud/spanner');
/**
* TODO(developer): Uncomment the following lines before running the sample.
*/
// const projectId = 'my-project-id';
// const instanceId = 'my-instance';
// const databaseId = 'my-database';
// Creates a client
const spanner = new Spanner({
projectId: projectId,
});
// Gets a reference to a Cloud Spanner instance and database
const instance = spanner.instance(instanceId);
const database = instance.database(databaseId);
database.runTransaction(async (err, transaction) => {
if (err) {
console.error(err);
return;
}
try {
const [rowCount] = await transaction.runUpdate({
sql:
'INSERT Singers (SingerId, FirstName, LastName) VALUES (10, @firstName, @lastName)',
params: {
firstName: 'Virginia',
lastName: 'Watson',
},
});
console.log(
`Successfully inserted ${rowCount} record into the Singers table.`
);
await transaction.commit();
} catch (err) {
console.error('ERROR:', err);
} finally {
// Close the database when finished.
database.close();
}
});
// [END spanner_dml_standard_insert]
}
.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about async, currently I am investigating on it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue turns up purely when you set the gaxOptions. If you don't, the sample runs fine.

Also, you can't set a subset of the fields. You have to set all of them.

  • first I set only the retry codes then it said ERROR: TypeError: Cannot read property 'retryDelayMultiplier' of undefined.
  • then I set retryDelayMultiplier then it said Error: 2 UNKNOWN: Getting metadata from plugin failed with error: Deadline is too far in the future
  • until you set each field, it keeps producing the same error.
  • once you set all of them, it errors with ERROR: TypeError: stream.on is not a function.

So it has something to do with the fact that this is a streaming RPC. We set gaxOptions in the admin RPC calls in the system tests and this doesn't happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skuruppu @hengfengli something with streaming in google-gax , I did check this in bigtable and found same error with stream.

Copy link
Contributor Author

@laljikanjareeya laljikanjareeya Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, we do not support retry in stream, Here is the issue in google-gax.
I will change the sample accordingly.

@hengfengli hengfengli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 19, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 19, 2020
@hengfengli
Copy link
Contributor

Hi @laljikanjareeya, is there any news for this?

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1210   +/-   ##
=======================================
  Coverage   98.24%   98.24%           
=======================================
  Files          23       23           
  Lines       21140    21140           
  Branches     1181     1181           
=======================================
  Hits        20769    20769           
  Misses        368      368           
  Partials        3        3           

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 2bbd6c8...7696a22. Read the comment docs.

@skuruppu
Copy link
Contributor

Please also wait for approval by @hengfengli since the sample now looks different to the one in Go.

Copy link
Contributor

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍 Thanks for doing this.

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label Aug 21, 2020
@laljikanjareeya laljikanjareeya merged commit f2f8827 into googleapis:master Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add code sample on setting per-request gaxOptions
4 participants