feat: add starting/max rates to BulkWriterOptions #1305

thebrianchen · 2020-09-22T20:42:00Z

Following up from the 555 discussion by adding option to set the initial and maximum rate of the throttler in BulkWriter.

codecov · 2020-09-22T21:28:43Z

Codecov Report

Merging #1305 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1305      +/-   ##
==========================================
+ Coverage   98.46%   98.51%   +0.04%     
==========================================
  Files          30       30              
  Lines       18648    18754     +106     
  Branches     1435     1450      +15     
==========================================
+ Hits        18362    18475     +113     
+ Misses        283      276       -7     
  Partials        3        3

Impacted Files	Coverage Δ
dev/src/bulk-writer.ts	`99.75% <100.00%> (+0.85%)`	⬆️
dev/src/index.ts	`97.57% <100.00%> (+0.06%)`	⬆️
dev/src/rate-limiter.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 67b0aba...5fa80f8. Read the comment docs.

dev/src/bulk-writer.ts

schmidt-sebastian · 2020-09-23T18:54:19Z

dev/src/bulk-writer.ts

+    }
+  }
+
+  if (


We can use validateInteger with a NumericRangeOptions to make sure that the provided numbers are >= 0.

Much better, done.

schmidt-sebastian · 2020-09-23T18:56:12Z

dev/src/bulk-writer.ts

+    if (typeof options.disableThrottling !== 'boolean') {
+      throw new Error(
+        `${invalidArgumentMessage(
+          arg,
+          'bulkWriter() options argument'
+        )} "disableThrottling" is not a boolean.`
+      );
+    }


We don't need to validate invalid TypeScript usage. You should only validate API input that is valid to the transpiler, but not to the SDK (such as disableThrottling and initialOpsPerSecond set together)

schmidt-sebastian · 2020-09-23T18:58:47Z

dev/test/bulk-writer.ts

+  describe('options', () => {
+    it('requires object', async () => {
+      const firestore = await createInstance();
+      expect(() => firestore.bulkWriter(42 as InvalidApiUsage)).to.throw(


You should be able to use //@ ts-expect-error now: https://dev.to/oliverjumpertz/making-good-use-of-ts-expect-error-in-typescript-1f41

I'll try to keep this in mind for next time! I'll probably end up cutting out the validation code at some point, which means we won't have any InvalidApiUsage instances that would otherwise require the expect-error comment.

schmidt-sebastian · 2020-09-23T19:04:27Z

types/firestore.d.ts

    readonly disableThrottling?: boolean;
+


A lot of the APIs I have been dealing with lately do the following:

export interface BulkWriterOptions { readonly throttling?: boolean | {initialOpsPerSecond?: number; maxOpsPerSecond?: number; }; }

This would match terser and eslint, but we might want to gather more feedback (maybe from @Feiyang1).

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

thebrianchen

Used the union types for BulkWriterOptions. I think my constructor logic could be cleaner -- please advise :)

I'll assign to Fei after you think it's in a good place.

thebrianchen · 2020-09-23T22:05:05Z

dev/src/bulk-writer.ts

+    }
+  }
+
+  if (


Much better, done.

thebrianchen · 2020-09-23T22:07:26Z

dev/test/bulk-writer.ts

+  describe('options', () => {
+    it('requires object', async () => {
+      const firestore = await createInstance();
+      expect(() => firestore.bulkWriter(42 as InvalidApiUsage)).to.throw(


I'll try to keep this in mind for next time! I'll probably end up cutting out the validation code at some point, which means we won't have any InvalidApiUsage instances that would otherwise require the expect-error comment.

schmidt-sebastian · 2020-09-24T16:36:22Z

dev/src/bulk-writer.ts

+      const startingRate =
+        typeof options?.throttling !== 'boolean' &&
+        options?.throttling?.initialOpsPerSecond !== undefined
+          ? options.throttling.initialOpsPerSecond
+          : STARTING_MAXIMUM_OPS_PER_SECOND;


I think this is:

Suggested change

const startingRate =

typeof options?.throttling !== 'boolean' &&

options?.throttling?.initialOpsPerSecond !== undefined

? options.throttling.initialOpsPerSecond

: STARTING_MAXIMUM_OPS_PER_SECOND;

const startingRate = options?.throttling?.initialOpsPerSecond ??

STARTING_MAXIMUM_OPS_PER_SECOND;

I tested it with this code:

const STARTING_MAXIMUM_OPS_PER_SECOND = 1337; const options1 : any = {throttling:false}; const options2 : any = {throttling:true}; const options3 : any = {throttling:{initialOpsPerSecond:10}}; function printThrottler(options?:any) { console.log(options?.throttling?.initialOpsPerSecond ??STARTING_MAXIMUM_OPS_PER_SECOND) } printThrottler(options1); printThrottler(options2); printThrottler(options3);

This prints 1337, 1337, 10.

Same below.

Since it's a union type, initialOpsPerSecond is not a number unless I explicity check it's not a boolean first.

With the additional change to lower the initial rate to match the max rate if it's below the default, I refactored it to use if statements, which is hopefully more readable.

schmidt-sebastian · 2020-09-24T16:37:12Z

dev/src/bulk-writer.ts

+ * Validates the use of 'value' as BulkWriterOptions.
+ *
+ * @private
+ * @param arg The argument name or argument index (for varargs methods).


You can probably be more specific since this is only used for BulkWriterOptions.

removed used of arg as a parameter.

schmidt-sebastian · 2020-09-24T16:37:29Z

dev/src/bulk-writer.ts

+ * @param value The object to validate.
+ * @throws if the input is not a valid BulkWriterOptions object.
+ */
+function validateBulkWriterOptions(arg: string | number, value: unknown): void {


This can just be a string.

schmidt-sebastian · 2020-09-24T16:38:37Z

dev/src/bulk-writer.ts

+      'initialOpsPerSecond',
+      options.throttling.initialOpsPerSecond,
+      {
+        minValue: 0,


This needs to be 1, doesn't it?

schmidt-sebastian · 2020-09-24T16:38:59Z

dev/src/bulk-writer.ts

+
+  if ('maxOpsPerSecond' in options.throttling) {
+    validateInteger('maxOpsPerSecond', options.throttling.maxOpsPerSecond, {
+      minValue: 0,


0 also doesn't quite work here.

schmidt-sebastian · 2020-09-24T16:40:29Z

dev/src/bulk-writer.ts

+    );
+  }
+
+  if ('maxOpsPerSecond' in options.throttling) {


If you change this to options.throttling.maxOpsPerSecond !== undefined, you can get rid of the bang below.

schmidt-sebastian · 2020-09-24T16:40:53Z

dev/src/bulk-writer.ts

+    });
+
+    if (
+      'initialOpsPerSecond' in options.throttling &&


Same: options.throttling. initialOpsPerSecond !== undefined

schmidt-sebastian · 2020-09-24T16:42:57Z

dev/src/bulk-writer.ts

+      !('initialOpsPerSecond' in options.throttling) &&
+      STARTING_MAXIMUM_OPS_PER_SECOND > options.throttling.maxOpsPerSecond!
+    ) {
+      throw new Error(


This is pretty unexpected, especially since the default value is not known. We should instead adjust the starting value.

BTW, I think we should somehow squeeze DEFAULT into the name of STARTING_MAXIMUM_OPS_PER_SECOND, since the constant is no longer required to be the maximum.

schmidt-sebastian · 2020-09-24T16:45:06Z

types/firestore.d.ts

+     */
+    readonly throttling?:
+      | boolean
+      | {initialOpsPerSecond?: number; maxOpsPerSecond?: number};


Thanks for the update. Did you check with @Feiyang1 if he thinks this is a useable API?

This looks fine to me. Autocomplete will help people figure it out. Currently we are only describing the config params in the comment. I would make it clear in the comment that it can be either a boolean or a config object (boolean to enable/disable, config to enable and set config values).

Was going to ask him after, but he jumped in :)

Do you also want to update the comment per Fei's suggestion?

I thought I had already added some more to the comments, but this time around I also explicitly stated that setting it to false disables throttling.

…into bc/bulk-555

schmidt-sebastian · 2020-09-24T23:30:56Z

dev/src/bulk-writer.ts

@@ -351,22 +356,46 @@ export class BulkWriter {

  constructor(
    private readonly firestore: Firestore,
-    enableThrottling: boolean
+    private readonly options?: firestore.BulkWriterOptions


Suggested change

private readonly options?: firestore.BulkWriterOptions

options?: firestore.BulkWriterOptions

schmidt-sebastian · 2020-09-24T23:33:14Z

types/firestore.d.ts

+     */
+    readonly throttling?:
+      | boolean
+      | {initialOpsPerSecond?: number; maxOpsPerSecond?: number};


Do you also want to update the comment per Fei's suggestion?

thebrianchen requested review from a team as code owners September 22, 2020 20:42

thebrianchen self-assigned this Sep 22, 2020

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

thebrianchen requested a review from schmidt-sebastian September 22, 2020 22:19

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Sep 22, 2020

feat: add starting/max rates to BulkWriterOptions

a34c278

thebrianchen force-pushed the bc/bulk-555 branch from 14c5e88 to a34c278 Compare September 22, 2020 22:40

product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Sep 23, 2020

schmidt-sebastian reviewed Sep 23, 2020

View reviewed changes

schmidt-sebastian removed their assignment Sep 23, 2020

Brian Chen and others added 5 commits September 23, 2020 14:16

Update dev/src/bulk-writer.ts

3bab963

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

Update dev/src/bulk-writer.ts

14bcf8e

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

address comment

4855291

use union type in BulkWriterOptions

c07f452

Merge branch 'master' into bc/bulk-555

d04839e

thebrianchen commented Sep 24, 2020

View reviewed changes

thebrianchen requested a review from schmidt-sebastian September 24, 2020 15:09

thebrianchen assigned thebrianchen and schmidt-sebastian and unassigned thebrianchen Sep 24, 2020

schmidt-sebastian reviewed Sep 24, 2020

View reviewed changes

schmidt-sebastian removed their assignment Sep 24, 2020

Brian Chen added 3 commits September 24, 2020 14:38

resolve comments after union types

8796906

Merge branch 'bc/bulk-555' of github.com:googleapis/nodejs-firestore …

024fe08

…into bc/bulk-555

add specifying comment

e092afa

thebrianchen requested a review from schmidt-sebastian September 24, 2020 19:52

thebrianchen assigned schmidt-sebastian Sep 24, 2020

schmidt-sebastian approved these changes Sep 24, 2020

View reviewed changes

Brian Chen added 2 commits September 24, 2020 20:43

final comments hopefully

917552a

one more comment update

5fa80f8

thebrianchen merged commit 57dcf1c into master Sep 25, 2020

thebrianchen deleted the bc/bulk-555 branch September 25, 2020 14:52

thebrianchen mentioned this pull request Oct 2, 2020

feat: add max/min throttling options to BulkWriterOptions googleapis/java-firestore#400

Merged

+                  }
+                }
+                if (

	private readonly options?: firestore.BulkWriterOptions
	options?: firestore.BulkWriterOptions

feat: add starting/max rates to BulkWriterOptions #1305

feat: add starting/max rates to BulkWriterOptions #1305

Conversation

thebrianchen commented Sep 22, 2020

codecov bot commented Sep 22, 2020 • 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

schmidt-sebastian Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebrianchen left a comment • 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

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

Choose a reason for hiding this comment

codecov bot commented Sep 22, 2020 •

edited

Loading

schmidt-sebastian Sep 23, 2020 •

edited

Loading

thebrianchen left a comment •

edited

Loading