fix: retry ABORTED writes in bulkCommit by thebrianchen · Pull Request #1243 · googleapis/nodejs-firestore

thebrianchen · 2020-06-25T21:41:22Z

Recreating #1222 to merge against PR without deleting comment history.

codecov · 2020-06-25T21:47:40Z

Codecov Report

Merging #1243 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1243      +/-   ##
==========================================
- Coverage   98.50%   98.50%   -0.01%     
==========================================
  Files          30       30              
  Lines       18545    18624      +79     
  Branches     1422     1431       +9     
==========================================
+ Hits        18268    18345      +77     
- Misses        274      276       +2     
  Partials        3        3

Impacted Files	Coverage Δ
dev/src/bulk-writer.ts	`98.68% <100.00%> (-0.19%)`	⬇️
dev/src/write-batch.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 b83874c...aaa5fed. Read the comment docs.

schmidt-sebastian · 2020-06-26T05:22:03Z

dev/src/bulk-writer.ts

-import {Deferred, wrapError} from './util';
+import {Deferred, getRetryCodes, wrapError} from './util';
 import {BatchWriteResult, WriteBatch, WriteResult} from './write-batch';
+import {Status} from 'google-gax';


Nit: External imports should go in the first block.

schmidt-sebastian · 2020-06-26T05:24:30Z

dev/src/bulk-writer.ts

+    error?: Error
+  ): number[] {
+    const indexesToRetry: number[] = [];
+    if (error) {


Can you add a small comment here to explain what this is doing?

schmidt-sebastian · 2020-06-26T06:18:33Z

dev/src/bulk-writer.ts

+      try {
+        const results = await this.writeBatch.bulkCommit();
+        retryIndexes = this.processResults(originalIndexMap, results);
+      } catch (err) {


I tried to see if I could get rid of this re-assignment and got completely carried away. Here is a cleanup:

https://gist.github.com/schmidt-sebastian/702ba33ce0c3515d096089226a762934

Use at your own risk and take at your own consideration :)

Huge fan of this organization. I implemented what you did, but changed keys to docPaths since I was a bit confused about what the keys were at first. I think this approach will work with the UpdateBuilder pattern on java as well.

schmidt-sebastian · 2020-06-26T06:22:46Z

dev/src/bulk-writer.ts

-    this.completedDeferred.resolve();
+
+    if (indexesToRetry.length === 0) {
+      this.completedDeferred.resolve();


We also need to resolve this when all retry attempts failed. Please also add a test for this.

schmidt-sebastian

Thanks for the update. Some naming/comment nits to ruin your weekend.

schmidt-sebastian · 2020-06-26T22:40:09Z

dev/src/bulk-writer.ts

+        results = await this.writeBatch.bulkCommit();
+      } catch (err) {
+        // Map the failure to each individual write's result.
+        results = [...this.pendingOps.keys()].map(key => {


Should "key" be "path" then?

schmidt-sebastian · 2020-06-26T22:41:37Z

dev/src/bulk-writer.ts

-    } else {
-      for (let i = 0; i < this.opCount; i++) {
-        this.resultsMap.get(i)!.reject(error);
+  processResults(results: BatchWriteResult[], shouldRetry: boolean): void {


s/shouldRetry/mayRetry (or allowRetry)?

Also might be worth adding @param comments.

N/A since we're using failRemainingOperations.

schmidt-sebastian · 2020-06-26T22:43:40Z

dev/src/bulk-writer.ts

+      }
+    }
+
+    this.processResults(results, /* shouldRetry= */ false);


This might be cleaner if this was this.failRemainingOperations(results) and a separate call.

I originally didn't want to copy/paste code, but in this case, it clarifies the behavior much better.

schmidt-sebastian · 2020-06-26T22:44:25Z

dev/src/bulk-writer.ts

+        this.completedDeferred.resolve();
+        return;


This could just be a break, this.processResult() would be a no-op.

The map entry is deleted from pendingOps after the promise is resolved in processResults, so an error would be thrown if processResults is called again.

schmidt-sebastian · 2020-06-26T22:46:19Z

dev/src/bulk-writer.ts

+    return code !== undefined && retryCodes.includes(code);
+  }
+
+  has(path: string): boolean {


Let's use a more descriptive name here (hasDoc(), hasWrite(), ...).

schmidt-sebastian · 2020-06-26T22:47:03Z

dev/src/write-batch.ts

+  constructor(firestore: Firestore);
+  constructor(
+    firestore: Firestore,
+    retryBatch?: WriteBatch,


Please document.

schmidt-sebastian · 2020-06-26T22:47:10Z

dev/src/write-batch.ts

+  constructor(
+    firestore: Firestore,
+    retryBatch?: WriteBatch,
+    docsToRetry?: string[]


Please document.

fix: retry ABORTED writes in bulkCommit

c05079d

thebrianchen self-assigned this Jun 25, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 25, 2020

thebrianchen requested a review from schmidt-sebastian June 25, 2020 21:49

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jun 25, 2020

thebrianchen mentioned this pull request Jun 25, 2020

fix: retry ABORTED writes in bulkCommit #1222

Closed

schmidt-sebastian reviewed Jun 26, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jun 26, 2020

schmidt-sebastian reviewed Jun 26, 2020

View reviewed changes

Brian Chen added 3 commits June 26, 2020 11:34

checkpoint before rewritingc

15e2abc

implement refactor to remove need for indexes

d2bb56e

add back readonly to _ops

d0aaf3d

thebrianchen requested a review from schmidt-sebastian June 26, 2020 22:04

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jun 26, 2020

quick slim

7592141

schmidt-sebastian approved these changes Jun 26, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and thebrianchen and unassigned schmidt-sebastian and wu-hui Jun 26, 2020

Brian Chen added 2 commits June 29, 2020 11:16

resolve final comments, add failRemainingOps

22d2dca

Merge branch 'master' into bc/retry-failed-writes-2

aaa5fed

thebrianchen merged commit 6007e43 into master Jun 29, 2020

thebrianchen deleted the bc/retry-failed-writes-2 branch June 29, 2020 18:44

thebrianchen mentioned this pull request Jul 7, 2020

fix: add retry on ABORTED errors googleapis/java-firestore#286

Merged

Conversation

thebrianchen commented Jun 25, 2020

Uh oh!

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

Uh oh!

Codecov Report

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jun 25, 2020 •

edited

Loading