chore: simplify Bulk Writer #1379

schmidt-sebastian · 2020-12-15T03:32:57Z

This is my attempt to simplify some of the callback logic in BulkWriter, and will hopefully allow us add Backoff handling for retries pretty easily.

The overall changes are:

There are no more batch queues. Instead, there is only ever a single batch. If this batch reaches capacity, it is send.
To allow 'flush' to block, I am instead keeping a list of pending operations.
An operation can go from "sent" to "ready_to_send" if the retry mechanism kicks in.

One behavior change is that it is now possible that a retry will get scheduled after other operations in a flush. Let's say we have the calls:

Op1
Op2
Flush
Op3

And Op1 fails. The write order could now be: Op1, Op2, Op3, Op1. This breaks a single unit test, but I think it is sane behavior as it allows us to make progress while we wait for more reponses.

codecov · 2020-12-15T17:01:52Z

Codecov Report

Merging #1379 (2853274) into master (7ff2345) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1379      +/-   ##
==========================================
- Coverage   98.52%   98.51%   -0.01%     
==========================================
  Files          32       32              
  Lines       19464    19341     -123     
  Branches     1374     1357      -17     
==========================================
- Hits        19176    19053     -123     
  Misses        284      284              
  Partials        4        4

Impacted Files	Coverage Δ
dev/src/bulk-writer.ts	`100.00% <100.00%> (+0.20%)`	⬆️
dev/src/write-batch.ts	`100.00% <100.00%> (ø)`
dev/src/rate-limiter.ts	`98.85% <0.00%> (-1.15%)`	⬇️

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 7ff2345...2853274. Read the comment docs.

schmidt-sebastian · 2020-12-16T15:51:23Z

Updated PR to remove state transitions (they are now handled internally in BulkCommitOperation). Also removed list of pending operations and instead added a pointer to the last scheduled operation.

thebrianchen

Wow, after taking the time to understand this, I am speechless. This is so sleek, so clean. Thanks for putting the time into this!

thebrianchen · 2020-12-17T23:33:17Z

dev/src/bulk-writer.ts

+  /**
+   * @param ref The document reference being written to.
+   * @param type The type of operation that created this write.
+   * @param op A closure that encapsulates the API call which adds this write to 


thebrianchen · 2020-12-17T23:38:58Z

dev/src/bulk-writer.ts

+  constructor(
+    public ref: firestore.DocumentReference<unknown>,
+    private type: 'create' | 'set' | 'update' | 'delete',
+    private op: (bulkCommitBatch: BulkCommitBatch) => void,


nit: add readonly where applicable here and below

Done (it's applicable everywhere)

thebrianchen · 2020-12-17T23:49:17Z

dev/src/bulk-writer.ts

-   * when the batch's response is received. This includes batches from both the
-   * batchQueue and retryBatchQueue.
+   * The batch that is currently used to schedule operations. Once this batch
+   * reached maximum capacity, a new batch is created.


nit: s/reached/reaches

thebrianchen · 2020-12-18T00:20:31Z

dev/src/bulk-writer.ts

+      // TODO(b/158502664): Use actual delete timestamp.
+      const DELETE_TIMESTAMP_SENTINEL = Timestamp.fromMillis(0);
+
+      const status = (response.status || [])[i];


In L216, we access status.code, but based on L208, status could be undefined (though it shouldn't normally happen). Should we add an assert statement before accessing status.code?

If the code is undefined, we pass it as undefined to the customer. That seems to be the reasonable thing to do, since we don't want to make up our own status code. But as you said, the status code should always be set.

thebrianchen · 2020-12-18T00:30:47Z

dev/src/bulk-writer.ts

  /**
-   * Sets the maximum number of allowed operations in a batch.
+   * Schedules the provided operations on current BulkCommitBatch.
+   * Sends the BulkCommitBatch if it reach maximum capacity.


s/reach/reaches

thebrianchen · 2020-12-18T00:38:55Z

dev/src/bulk-writer.ts

+      ref,
+      type,
+      op,
+      this._sendFn.bind(this),


Just curious, is there a behavioral difference between using bind here vs. using a lambda?

No, I was just hoping prettier wouldn't span this across 6 lines, but I was wrong. Now I am too lazy to go back.

thebrianchen · 2020-12-18T00:51:59Z

dev/src/bulk-writer.ts

-   * reference, or creates one if no eligible batches are found.
+   * Sends the current batch and resets `this._bulkCommitBatch`.
   *
+   * @param flush - If provided, keeps re-sending operations until no more


nit: remove -.

thebrianchen · 2020-12-18T00:57:32Z

dev/src/bulk-writer.ts

-    return this._rateLimiter;
+    op.run(this._bulkCommitBatch);
+    this._bulkCommitBatch.processLastOperation(op);
+    this._lastOp = this._lastOp.then(() => silencePromise(op.promise));


Can you add a comment here about advancing the pointer? It took me a bit to realize what was happening

thebrianchen · 2020-12-18T01:02:07Z

dev/src/bulk-writer.ts

+        pendingBatch._opCount
+      );
+      logger(
+        'BulkWriter.sendNextBatch',


s/sendNextBatch/sendCurrentBatch

Done. Thanks for catching.

schmidt-sebastian

Thanks for the review!

schmidt-sebastian · 2020-12-18T02:45:36Z

dev/src/bulk-writer.ts

+  /**
+   * @param ref The document reference being written to.
+   * @param type The type of operation that created this write.
+   * @param op A closure that encapsulates the API call which adds this write to 


schmidt-sebastian · 2020-12-18T02:45:40Z

dev/src/bulk-writer.ts

+  constructor(
+    public ref: firestore.DocumentReference<unknown>,
+    private type: 'create' | 'set' | 'update' | 'delete',
+    private op: (bulkCommitBatch: BulkCommitBatch) => void,


Done (it's applicable everywhere)

schmidt-sebastian · 2020-12-18T02:47:54Z

dev/src/bulk-writer.ts

+      // TODO(b/158502664): Use actual delete timestamp.
+      const DELETE_TIMESTAMP_SENTINEL = Timestamp.fromMillis(0);
+
+      const status = (response.status || [])[i];


If the code is undefined, we pass it as undefined to the customer. That seems to be the reasonable thing to do, since we don't want to make up our own status code. But as you said, the status code should always be set.

schmidt-sebastian · 2020-12-18T02:48:16Z

dev/src/bulk-writer.ts

-   * when the batch's response is received. This includes batches from both the
-   * batchQueue and retryBatchQueue.
+   * The batch that is currently used to schedule operations. Once this batch
+   * reached maximum capacity, a new batch is created.


schmidt-sebastian · 2020-12-18T02:48:57Z

dev/src/bulk-writer.ts

-   * reference, or creates one if no eligible batches are found.
+   * Sends the current batch and resets `this._bulkCommitBatch`.
   *
+   * @param flush - If provided, keeps re-sending operations until no more


schmidt-sebastian · 2020-12-18T02:49:16Z

dev/src/bulk-writer.ts

+        pendingBatch._opCount
+      );
+      logger(
+        'BulkWriter.sendNextBatch',


Done. Thanks for catching.

schmidt-sebastian · 2020-12-18T02:49:47Z

dev/src/bulk-writer.ts

+      ref,
+      type,
+      op,
+      this._sendFn.bind(this),


No, I was just hoping prettier wouldn't span this across 6 lines, but I was wrong. Now I am too lazy to go back.

schmidt-sebastian · 2020-12-18T02:50:04Z

dev/src/bulk-writer.ts

  /**
-   * Sets the maximum number of allowed operations in a batch.
+   * Schedules the provided operations on current BulkCommitBatch.
+   * Sends the BulkCommitBatch if it reach maximum capacity.


schmidt-sebastian · 2020-12-18T02:52:02Z

dev/src/bulk-writer.ts

-    return this._rateLimiter;
+    op.run(this._bulkCommitBatch);
+    this._bulkCommitBatch.processLastOperation(op);
+    this._lastOp = this._lastOp.then(() => silencePromise(op.promise));


miketierce · 2020-12-28T18:58:46Z

I can no longer create a new BulkWriter
const bulkWriter = new admin.firestore.BulkWriter()
presents this error
TypeError: Cannot read property '_incrementBulkWritersCount' of undefined

schmidt-sebastian · 2020-12-29T02:54:37Z

@miketierce This PR hasn't been released yet. If the issue persists, can you file an issue in this repo?

cc @thebrianchen

This is my attempt to simplify some of the callback logic in BulkWriter, and will hopefully allow us add Backoff handling for retries pretty easily. The overall changes are: - There are no more batch queues. Instead, there is only ever a single batch. If this batch reaches capacity, it is send. - To allow 'flush' to block, I am instead keeping a list of pending operations. One behavior change is that it is now possible that a retry will get scheduled after other operations in a flush. Let's say we have the calls: - Op1 - Op2 - Flush - Op3 And Op1 fails. The write order could now be: Op1, Op2, Op3, Op1. This breaks a single unit test, but I think it is sane behavior as it allows us to make progress while we wait for more reponses. Port of googleapis/nodejs-firestore#1379

schmidt-sebastian requested review from a team as code owners December 15, 2020 03:32

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

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

schmidt-sebastian force-pushed the mrschmidt/simplifybulkwriter branch from 28204df to 2900034 Compare December 15, 2020 16:55

schmidt-sebastian changed the title ~~Mrschmidt/simplifybulkwriter~~ chore: simplify Bulk Writer Dec 15, 2020

schmidt-sebastian force-pushed the mrschmidt/simplifybulkwriter branch from 2900034 to b480bf7 Compare December 15, 2020 17:08

chore: Simpify BulkWriter

dffa081

schmidt-sebastian force-pushed the mrschmidt/simplifybulkwriter branch from b480bf7 to dffa081 Compare December 15, 2020 17:22

schmidt-sebastian assigned thebrianchen Dec 15, 2020

schmidt-sebastian requested a review from thebrianchen December 15, 2020 17:23

schmidt-sebastian added 3 commits December 15, 2020 11:20

Add test back

13725bd

Use tail pointer

89fc15f

Merge branch 'master' into mrschmidt/simplifybulkwriter

8097c86

schmidt-sebastian added 6 commits December 16, 2020 08:55

Doc fixes

da71aba

Update write-batch.ts

5255bcd

Update bulk-writer.ts

cd9e38c

Deflake test

521cb3d

Update docs

687ad14

Docs

ab5c463

schmidt-sebastian force-pushed the mrschmidt/simplifybulkwriter branch from ca36340 to ab5c463 Compare December 17, 2020 01:02

Lint

d27f860

thebrianchen approved these changes Dec 18, 2020

View reviewed changes

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Dec 18, 2020

schmidt-sebastian commented Dec 18, 2020

View reviewed changes

Feedback

2853274

schmidt-sebastian merged commit f4899ce into master Dec 18, 2020

miketierce mentioned this pull request Dec 28, 2020

I can no longer create a new BulkWriter #1385

Closed

schmidt-sebastian mentioned this pull request Feb 3, 2021

chore: simplify BulkWriter googleapis/java-firestore#523

Merged

chore: simplify Bulk Writer #1379

chore: simplify Bulk Writer #1379

Uh oh!

Conversation

schmidt-sebastian commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

schmidt-sebastian commented Dec 16, 2020

Uh oh!

thebrianchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miketierce commented Dec 28, 2020

Uh oh!

schmidt-sebastian commented Dec 29, 2020

Uh oh!

Reviewers

Assignees

Labels

schmidt-sebastian commented Dec 15, 2020 •

edited

Loading

codecov bot commented Dec 15, 2020 •

edited

Loading