feat: support RPC priority #1282

olavloite · 2020-12-04T17:09:24Z

Adds support for setting RPC priority.
This PR also uses some of the changes that are introduced in #1254.

skuruppu · 2020-12-07T10:45:07Z

Also adding @AVaksman who is working on tagging which may touch similar code paths.

skuruppu · 2020-12-10T09:43:15Z

samples/rpc-priority.js

+          ORDER BY AlbumTitle`;
+
+  try {
+    const [rows] = await database.run({


It would be great to add a comment to say that we're explicitly lowering the priority of this query by setting this option.

skuruppu · 2020-12-10T09:47:35Z

src/transaction.ts

@@ -1253,15 +1261,15 @@ export class Transaction extends Dml {

  batchUpdate(
    queries: Array<string | Statement>,
-    gaxOptions?: CallOptions
+    options?: BatchUpdateOptions


Because of changes like this, we may actually have to make the type changes separately in time for the breaking change release we'll do in early January. Do you think that would be ok?

Yeah, I'll extract this change to a separate PR.

Following @JustinBeckwith's suggestion in #1278 (comment), we could also change this parameter (and others) to BatchUpdateOptions | CallOptions. The definition of this and the other methods would then look something like this:

batchUpdate( queries: Array<string | Statement>, optionsOrCallback?: BatchUpdateOptions | CallOptions | BatchUpdateCallback, cb?: BatchUpdateCallback ): Promise<BatchUpdateResponse> | void { const options = typeof optionsOrCallback === 'object' ? optionsOrCallback : {}; const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; const gaxOpts = 'gaxOptions' in options ? (options as BatchUpdateOptions).gaxOptions : options; ... }

(@JustinBeckwith Please feel free to correct me if I misunderstood your comment)

@JustinBeckwith Friendly ping on this. Would you mind giving us your input/opinion on the above?

I tried this out for CommitStats. It works to avoid the breaking change. But there is a caveat that @AVaksman pointed at which is that if options doesn't have gaxOptions then we pass the options as a whole even if it is not a CallOptions.

Is this ok?

I tried to use a Discriminating Union but then we need the user to specify the descriminator field, which would end up being a breaking change anyway.

I assume there is no other way to do this than the idea you suggested @olavloite . I'll be going ahead with this implementation for CommitStats. So if you could do the same in this PR, then that works.

Yeah - we really want to avoid breaking changes in these library unless it's unreasonable to avoid. We're starting to evaluate the possibility of backporting fixes to prior major releases, so the cost of these is going to go waaaaay up. I would recommend avoiding the break if you can.

I've changed the different options arguments to BatchUpdateOptions | CallOptions (and similar) and added type checking where necessary. That should make this a non-breaking change.

JustinBeckwith · 2021-01-19T05:53:44Z

samples/rpc-priority.js

+
+'use strict';
+
+async function queryWithRpcPriority(instanceId, databaseId, projectId) {


There are no region tags here, so these samples won't be usable within cloud.google.com. Should we add them?

Yes, eventually, but they are not known yet.

JustinBeckwith · 2021-01-19T05:56:22Z

samples/rpc-priority.js

+          ORDER BY AlbumTitle`;
+
+  try {
+    const [rows] = await database.run({


When using an async function like this, we expect a wrapping main function, and an inner async function that's inside the region tag. Here is a good example:
https://github.com/googleapis/nodejs-bigquery/blob/master/samples/createJob.js

For the full description of how these work and why, check out:
https://g3doc.corp.google.com/company/teams/cloud-devrel/dpe/samples/nodejs.md?cl=head

@skuruppu it looks like every sample in this repo needs to be checked for conformance against that rubric :/

Ack @JustinBeckwith, I didn't realize. We'll do this as a separate cleanup effort. Tracked in #1295.

Thanks for the information. I've updated this specific sample based on the example you provided. I unfortunately am not allowed to access the documentation that you linked.

JustinBeckwith · 2021-01-19T05:57:00Z

samples/rpc-priority.js

+
+    rows.forEach(row => {
+      const json = row.toJSON();
+      const marketingBudget = json.MarketingBudget


I'm curious - why is it important this value is null vs undefined or ''?

Good point, it's not (lazy copy-paste from a different sample). I removed that specific part.

JustinBeckwith · 2021-01-19T05:57:56Z

samples/rpc-priority.js

+  }
+}
+
+require('yargs')


With all of our new samples, we no longer use yargs and instead rely on positional parameters. See the rubric.

Changed to positional parameters.

JustinBeckwith · 2021-01-19T06:00:33Z

src/transaction.ts

@@ -1253,15 +1261,15 @@ export class Transaction extends Dml {

  batchUpdate(
    queries: Array<string | Statement>,
-    gaxOptions?: CallOptions
+    options?: BatchUpdateOptions


Yeah - we really want to avoid breaking changes in these library unless it's unreasonable to avoid. We're starting to evaluate the possibility of backporting fixes to prior major releases, so the cost of these is going to go waaaaay up. I would recommend avoiding the break if you can.

codecov · 2021-02-12T09:15:40Z

Codecov Report

Merging #1282 (5658ff4) into master (5659e40) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1282   +/-   ##
=======================================
  Coverage   98.60%   98.60%           
=======================================
  Files          23       23           
  Lines       21942    21960   +18     
  Branches     1229     1235    +6     
=======================================
+ Hits        21636    21654   +18     
  Misses        297      297           
  Partials        9        9

Impacted Files	Coverage Δ
src/index.ts	`100.00% <100.00%> (ø)`
src/transaction.ts	`100.00% <100.00%> (ø)`

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 5659e40...5658ff4. Read the comment docs.

AVaksman · 2021-03-25T21:13:07Z

src/transaction.ts

@@ -60,6 +61,7 @@ export interface RequestOptions {
 }

 export interface CommitOptions {
+  requestOptions?: IRequestOptions;


The IRequestOptions interface will contain RPC priority options and the tagging options. However CommitOptions should not support tagging options as a parameter.
I am suggesting either to define a separate interface with just a priority option or another way would be to use some ts magic

requestOptions?: Pick<IRequestOptions, 'priority'>;

I think we also need to think of a different name, since we have a same name parameter in ExecuteSqlRequest and BatchUpdateOptions interfaces, in which "requestOptions" will support priority and a requestTag.

perhaps "commitRequestOptions" and "transactionRequetOptions", any suggestions are most welcome.

I think doing something like:

requestOptions?: Omit<IRequestOptions, 'request_tag'>;

would be my preference.

I'm not sure about having to name the request options differently in each interface though. We want to keep our interfaces mirroring the protos as much possible. I think it's sufficient to use Pick/Omit to make it clear which of the IRequestOptions fields are available for use or not. In the worst case, we can just add documentation to make it clear which fields are supported.

AVaksman · 2021-03-25T21:40:17Z

src/transaction.ts

@@ -123,6 +126,7 @@ export interface BatchUpdateCallback {
    response?: spannerClient.spanner.v1.ExecuteBatchDmlResponse
  ): void;
 }
+export type BatchUpdateOptions = CommitOptions;


Also to have in mind that "requestOptions" (or "transactionRequetOptions" or better name) of BatchUpdateOptions will support requestTag in the future.

Once proto changes for tagging is merged something like this could be used

requestOptions?: Omit<IRequestOptions, 'transactionTag'>

I guess that means we have to explicitly create a BatchUpdateOptions interface as it's not the same as CommitOptions.

AVaksman · 2021-03-25T22:06:07Z

src/transaction.ts

@@ -76,6 +78,7 @@ export interface ExecuteSqlRequest extends Statement, RequestOptions {
  partitionToken?: Uint8Array | string;
  seqno?: number;
  queryOptions?: IQueryOptions;
+  requestOptions?: IRequestOptions;


Probably should not use the same property name as in CommitOptions interface since CommitOptions.requestOptions will only support priority parameter and in both ExecuteSqlRequest and BatchUpdateOptions same property name will support requestTag parameter

I think it's fine as long as we use Pick/Omit to make it clear which fields are supported/not supported.

skuruppu · 2021-03-30T22:30:00Z

I merged the codegen PR so once we rebase, we can continue the discussion.

skuruppu · 2021-04-01T01:20:32Z

@olavloite if you could make the changes such that this PR can be merged before #1286, that would be great.

olavloite · 2021-04-01T08:34:16Z

I merged the codegen PR so once we rebase, we can continue the discussion.

@skuruppu

I think that there's something strange going on with that codegen. The RequestOptions message has been added to the proto files, but it has not actually regenerated the proto Typescript and Javascript files. So the build is currently failing because it cannot find the RequestOptions definition.

It seems that the regenerate step is missing the actual protoc step. Any idea why?

AVaksman · 2021-04-01T12:31:35Z

I merged the codegen PR so once we rebase, we can continue the discussion.

@skuruppu

I think that there's something strange going on with that codegen. The RequestOptions message has been added to the proto files, but it has not actually regenerated the proto Typescript and Javascript files. So the build is currently failing because it cannot find the RequestOptions definition.

It seems that the regenerate step is missing the actual protoc step. Any idea why?

I've tried to regenerate locally and this is what I get

DEBUG:synthtool:Compiling protos...
(node:14631) UnhandledPromiseRejectionWarning: Error: ENOENT: no such file or directory, open 'protos/google/rpc/error_details.proto'
    at Object.openSync (fs.js:462:3)
    at Object.readFileSync (fs.js:364:35)
    at fetch (/www/nodejs-spanner/node_modules/protobufjs/src/root.js:172:34)
    at Root.load (/www/nodejs-spanner/node_modules/protobufjs/src/root.js:206:13)
    at Root.loadSync (/www/nodejs-spanner/node_modules/protobufjs/src/root.js:247:17)
    at main (/www/nodejs-spanner/node_modules/protobufjs/cli/pbjs.js:235:18)
    at internal/util.js:297:30
    at new Promise (<anonymous>)
    at main (internal/util.js:296:12)
    at compileProtos (/www/nodejs-spanner/node_modules/google-gax/build/tools/compileProtos.js:212:11)
(node:14631) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
(node:14631) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

skuruppu · 2021-04-06T04:50:51Z

Hmmmm I'm not sure what's going on. I asked the Node folks for help.

skuruppu · 2021-04-06T06:07:16Z

The issue should be fixed now, please rebase again.

olavloite · 2021-04-06T13:31:54Z

@skuruppu Thanks for getting the proto changes fixed and merged. It works now.

@AVaksman I've rebased and updated the PR based on our latest discussions. Would you mind taking a look and see if this would work for you as well for tagging?

AVaksman · 2021-04-07T10:42:39Z

src/transaction.ts

@@ -61,7 +61,7 @@ export interface RequestOptions {
 }

 export interface CommitOptions {
-  requestOptions?: IRequestOptions;
+  requestOptions?: Omit<IRequestOptions, 'requestTag'>;


I believe it should be

requestOptions?: Pick<IRequestOptions, 'priority'>;

here

No, I don't think that's correct. CommitOptions are used for blind writes such as database.table('some-table').insert(..), and those should support setting a transaction tag.

(After our conversation earlier today): I've removed transaction-tag from CommitOptions.

skuruppu · 2021-04-08T00:15:58Z

samples/rpc-priority.js

+    projectId: projectId,
+  });
+
+  async function queryWithRpcPriority(instanceId, databaseId) {


I forgot to mention this earlier. There are no code samples for this feature that will get pulled in from docs. So there's a question of whether we should remove the sample and just add an integration test. There's no way to test if the priority is being accepted by the backend but at least you're able to set it.

What do you think?

skuruppu · 2021-04-08T00:16:16Z

samples/system-test/spanner.test.js

+  // TODO: Enable when RPC Priority has been released.
+  it.skip('should use request options', async () => {


If we choose to keep the sample then you can unskip this now.

Argh. I missed this one and went ahead and merged without it. I've opened a new PR for enabling it here: #1338

olavloite added 2 commits December 4, 2020 18:05

regen: regenerate client with rpc priority

1c57cc0

feat: support rpc priority

1c0103d

olavloite added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 4, 2020

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 4, 2020

product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label Dec 4, 2020

olavloite changed the title ~~[WIP] feat: support RPC priority~~ feat: support RPC priority Dec 4, 2020

olavloite requested a review from skuruppu December 4, 2020 17:29

olavloite marked this pull request as ready for review December 4, 2020 17:29

olavloite requested review from a team as code owners December 4, 2020 17:29

skuruppu requested a review from AVaksman December 7, 2020 10:44

skuruppu approved these changes Dec 10, 2020

View reviewed changes

skuruppu mentioned this pull request Dec 11, 2020

feat!: restore() now accepts RestoreOptions #1278

Closed

skuruppu mentioned this pull request Jan 7, 2021

feat: CommitStats in CommitResponse #1254

Merged

JustinBeckwith reviewed Jan 19, 2021

View reviewed changes

olavloite added 2 commits January 19, 2021 12:19

fix: change to non-breaking change + fix sample

fa79af5

docs: cleanup sample and add comment on prio

26104cb

olavloite requested a review from JustinBeckwith January 19, 2021 11:47

olavloite added 2 commits January 25, 2021 11:31

fix: update copyright year

c2417db

Merge branch 'master' into rpc-priority

ccb2d93

olavloite added 3 commits February 12, 2021 10:16

chore: cleanup after merge

994d81b

fix: sample test + possible undefined requestoptions

a4138c0

fix: wait for sample to finish

28bfeb9

olavloite mentioned this pull request Feb 26, 2021

feat: add tagging support #1286

Closed

3 tasks

AVaksman reviewed Mar 25, 2021

View reviewed changes

Merge branch 'master' into rpc-priority

631f25c

olavloite added 2 commits March 31, 2021 13:50

Merge branch 'master' into rpc-priority

ceeb480

merge: remove manual changes from generated files

5fd78fe

Merge branch 'master' into rpc-priority

153fc11

Merge branch 'master' into rpc-priority

1ccb66b

olavloite removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 6, 2021

olavloite added 2 commits April 6, 2021 14:34

fix: omit transaction tags + add tests

c1abb8f

fix: skip notation

384fa40

AVaksman reviewed Apr 7, 2021

View reviewed changes

olavloite added 2 commits April 7, 2021 20:22

fix: CommitOptions should not include transaction tag

439b7d6

Merge branch 'master' into rpc-priority

5658ff4

skuruppu approved these changes Apr 8, 2021

View reviewed changes

olavloite merged commit 8c82694 into googleapis:master Apr 8, 2021

olavloite deleted the rpc-priority branch April 8, 2021 05:48


		'use strict';

		async function queryWithRpcPriority(instanceId, databaseId, projectId) {

		// TODO: Enable when RPC Priority has been released.
		it.skip('should use request options', async () => {

feat: support RPC priority #1282

feat: support RPC priority #1282

Conversation

olavloite commented Dec 4, 2020 • edited by skuruppu

skuruppu commented Dec 7, 2020

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

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

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

codecov bot commented Feb 12, 2021 • edited

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

Choose a reason for hiding this comment

skuruppu commented Mar 30, 2021

skuruppu commented Apr 1, 2021

olavloite commented Apr 1, 2021

AVaksman commented Apr 1, 2021

skuruppu commented Apr 6, 2021

skuruppu commented Apr 6, 2021

olavloite commented Apr 6, 2021

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

olavloite commented Dec 4, 2020 •

edited by skuruppu

codecov bot commented Feb 12, 2021 •

edited