fix: BulkWriter: add backoff on retries by thebrianchen · Pull Request #1447 · googleapis/nodejs-firestore

thebrianchen · 2021-03-10T23:20:04Z

No description provided.

codecov · 2021-03-10T23:26:02Z

Codecov Report

Merging #1447 (dd00707) into master (a36bb09) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1447      +/-   ##
==========================================
+ Coverage   98.20%   98.22%   +0.01%     
==========================================
  Files          32       32              
  Lines       19509    19565      +56     
  Branches     1276     1283       +7     
==========================================
+ Hits        19159    19217      +58     
+ Misses        346      344       -2     
  Partials        4        4

Impacted Files	Coverage Δ
dev/src/backoff.ts	`98.29% <100.00%> (ø)`
dev/src/bulk-writer.ts	`100.00% <100.00%> (ø)`
dev/src/rate-limiter.ts	`100.00% <0.00%> (+1.14%)`	⬆️

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 a36bb09...dd00707. Read the comment docs.

dev/src/bulk-writer.ts

schmidt-sebastian · 2021-03-11T18:48:44Z

dev/src/bulk-writer.ts

  // An array of pending write operations. Only contains writes that have not
  // been resolved.
-  private pendingOps: Array<BulkWriterOperation> = [];
+  private _pendingOps: Array<BulkWriterOperation> = [];


This can just be renamed to pendingOps and made readonly.

schmidt-sebastian · 2021-03-11T18:51:45Z

dev/src/bulk-writer.ts

+    if (failureCount === 0) return 0;
+    if (useMaxDelay) return DEFAULT_BACKOFF_MAX_DELAY_MS;


While this is correct for our use case, this reads a bit strange. I would flip these two statements. That means that we would end up using useMaxDelay no matter what the failure count is, which I think is easier to understand.

schmidt-sebastian · 2021-03-11T18:55:23Z

dev/src/bulk-writer.ts

+    return Math.min(
+      DEFAULT_BACKOFF_MAX_DELAY_MS,
+      backoffMs + Math.random() * 0.3 * backoffMs
+    );


Applying the jitter to the entire duration of the backoff creates a pretty strong variance, and is different from how we compute the backoff jitter elsewhere. Can we use the last backoff and then only apply the jitter to the next section? This would either be:

DEFAULT_BACKOFF_INITIAL_DELAY_MS * Math.pow(DEFAULT_BACKOFF_FACTOR, failureCount-1) + (DEFAULT_BACKOFF_FACTOR * DEFAULT_BACKOFF_INITIAL_DELAY_MS * jitter); (Maybe?)

or a simple backoff time in Delayed operation.

If I understand correctly, the ExponentialBackoff class has even more variance. We use currentBaseMs + (Math.random-0.5)*currentBaseMs, where currentBaseMs = initialDelay * pow(1.5, numAttempts).

I modeled the BulkWriter approach after ExponentialBackoff, and used 0.3 instead of 0.5 to ensure that the backoff increases each time.

This is bit more of a nit (and it probably won't affect performance), but I feel that adding the delay with jitter rather than multiplying it goes against the spirit of exponential backoff, because 100x^k != 100x^(k-1)+x

schmidt-sebastian · 2021-03-11T18:59:18Z

dev/src/bulk-writer.ts

+  get failedAttempts(): number {
+    return this._failedAttempts;
+  }
+


Some thoughts/ideas:

Could each operation calculate its own backoff? We can then use a simpler calculation where each backoff depends on the previous backoff time (including jitter). You would just keep a simple "last_backoff_duration" in BulkWriterOperation and multiple it per failed attempt.

Would it be possible to use the Backoff class? We should just need to expose a method to return the backoff duration.

I think it would be cleaner if we stored a lastError instead of the more specific resourceExhausted. It is cleaner and more versatile for later modifications.

Could each operation calculate its own backoff?

Done.

Would it be possible to use the Backoff class?

See my below comment on how the backoff used here is different than ExponentialBackoff. I don't think there is much to reuse in terms of logic here.

I think it would be cleaner if we stored a lastError instead of the more specific resourceExhausted.

Done.

schmidt-sebastian · 2021-03-11T19:00:08Z

dev/test/bulk-writer.ts

+    });
+  });
+
+  it('applies maximum backoff on retries for RESOURCE_EXHAUSTED', async () => {


Let's also add a test that uses multiple different backoffs in a batch and a test that verifies that we send a second batch before the first batch if the first batch has a backoff.

schmidt-sebastian · 2021-03-12T22:21:59Z

dev/src/bulk-writer.ts

+/*!
+ * The default jitter to apply. For example, a factor of 0.3 means a 30% jitter
+ * is applied.
+ */


This should mention that this relates to backoff. It needs some context in this module.

schmidt-sebastian · 2021-03-12T22:22:32Z

dev/src/bulk-writer.ts

  private deferred = new Deferred<WriteResult>();
-  private failedAttempts = 0;
+  private _failedAttempts = 0;
+  private _lastError?: Status;


lastStatus?

schmidt-sebastian · 2021-03-12T22:24:25Z

dev/src/bulk-writer.ts

    return this.deferred.promise;
  }

+  get lastError(): Status | undefined {


I am surprised this need to be exported.

Me too, removed.

schmidt-sebastian · 2021-03-12T22:25:47Z

dev/src/bulk-writer.ts

-      const delayMs = this._rateLimiter.getNextRequestDelayMs(
-        pendingBatch._opCount
-      );
+      const finalDelayMs = Math.max(backoffMsWithJitter, delayMs);


You can move out of the if statement and then use it in the if(...) check itself.

dev/src/bulk-writer.ts

schmidt-sebastian · 2021-03-12T22:30:26Z

dev/src/bulk-writer.ts

+    const jitter =
+      Math.random() *
+      DEFAULT_JITTER_FACTOR *
+      (Math.round(Math.random()) ? 1 : -1);


Math.random() is not cheap. You can remove one call: Math.random() * 2 * DEFAULT_JITTER_FACTOR - DEFAULT_JITTER_FACTOR.

Ooh that's some nice mathing! Refactored it slightly to make it more understandable (at least to me).

schmidt-sebastian · 2021-03-12T22:37:50Z

dev/test/bulk-writer.ts

+        (1 - DEFAULT_JITTER_FACTOR) * expected[timeoutHandlerCounter],
+        (1 + DEFAULT_JITTER_FACTOR) * expected[timeoutHandlerCounter]
+      );
+      timeoutHandlerCounter++;


schmidt-sebastian · 2021-03-15T18:10:33Z

dev/src/bulk-writer.ts

 class BulkWriterOperation {
  private deferred = new Deferred<WriteResult>();
-  private failedAttempts = 0;
+  private _failedAttempts = 0;


You can rename this back to its old name since there is no getter anymore.

dev/src/bulk-writer.ts

fix: BulkWriter: add backoff on retries

f732568

thebrianchen self-assigned this Mar 10, 2021

thebrianchen requested review from a team March 10, 2021 23:20

product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Mar 10, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 10, 2021

thebrianchen commented Mar 10, 2021

View reviewed changes

dev/src/bulk-writer.ts Outdated Show resolved Hide resolved

thebrianchen requested a review from schmidt-sebastian March 10, 2021 23:31

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Mar 10, 2021

schmidt-sebastian reviewed Mar 11, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Mar 11, 2021

resolve comments rd. 1

f47f92a

thebrianchen requested a review from schmidt-sebastian March 12, 2021 20:21

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Mar 12, 2021

schmidt-sebastian reviewed Mar 12, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Mar 12, 2021

resolve comments rd. 2

bcfb0d4

thebrianchen requested a review from schmidt-sebastian March 15, 2021 15:58

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Mar 15, 2021

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Mar 15, 2021

move tryMakeRequest away from assert

9cbbecf

schmidt-sebastian reviewed Mar 15, 2021

View reviewed changes

schmidt-sebastian approved these changes Mar 15, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned thebrianchen Mar 15, 2021

s/_failedAttempts/failedAttempts

dd00707

thebrianchen merged commit f483083 into master Mar 15, 2021

thebrianchen deleted the bc/bulk-backoff branch March 15, 2021 18:46

thebrianchen restored the bc/bulk-backoff branch March 15, 2021 18:47

thebrianchen deleted the bc/bulk-backoff branch March 15, 2021 18:47

thebrianchen mentioned this pull request Apr 20, 2021

feat: add backoff to BulkWriter googleapis/java-firestore#600

Merged

		if (failureCount === 0) return 0;
		if (useMaxDelay) return DEFAULT_BACKOFF_MAX_DELAY_MS;

Conversation

thebrianchen commented Mar 10, 2021

Uh oh!

codecov bot commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Mar 10, 2021 •

edited

Loading