chore: simplify BulkWriter #523

schmidt-sebastian · 2021-02-03T02:40:32Z

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

codecov · 2021-02-03T19:07:33Z

Codecov Report

Merging #523 (ee7a945) into master (0e6f3da) will decrease coverage by 0.27%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #523      +/-   ##
============================================
- Coverage     74.09%   73.81%   -0.28%     
+ Complexity     1117     1080      -37     
============================================
  Files            66       67       +1     
  Lines          5883     5801      -82     
  Branches        723      706      -17     
============================================
- Hits           4359     4282      -77     
  Misses         1296     1296              
+ Partials        228      223       -5

Impacted Files	Coverage Δ	Complexity Δ
...n/java/com/google/cloud/firestore/Transaction.java	`85.41% <ø> (ø)`	`13.00 <0.00> (ø)`
...in/java/com/google/cloud/firestore/WriteBatch.java	`100.00% <ø> (ø)`	`3.00 <0.00> (ø)`
...va/com/google/cloud/firestore/BulkCommitBatch.java	`100.00% <100.00%> (+6.94%)`	`7.00 <4.00> (-11.00)`	⬆️
...in/java/com/google/cloud/firestore/BulkWriter.java	`100.00% <100.00%> (+1.75%)`	`42.00 <9.00> (-12.00)`	⬆️
...om/google/cloud/firestore/BulkWriterOperation.java	`100.00% <100.00%> (ø)`	`7.00 <7.00> (?)`
...java/com/google/cloud/firestore/UpdateBuilder.java	`94.97% <100.00%> (ø)`	`59.00 <0.00> (ø)`
...a/com/google/cloud/firestore/BatchWriteResult.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-3.00%)`
...n/java/com/google/cloud/firestore/RateLimiter.java	`95.00% <0.00%> (-5.00%)`	`14.00% <0.00%> (-1.00%)`
...oogle/cloud/firestore/v1/FirestoreAdminClient.java	`55.97% <0.00%> (-1.50%)`	`38.00% <0.00%> (ø%)`
...a/com/google/cloud/firestore/DocumentSnapshot.java	`80.85% <0.00%> (-0.87%)`	`39.00% <0.00%> (ø%)`
... and 8 more

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 0e6f3da...ee7a945. Read the comment docs.

thebrianchen

Thanks for porting this! Unfortunately, the unit tests flake when you run them until failure on IntelliJ. I suspect this might be because my old race conditions were covered by using MoreExecutors.directExecutor() instead of calling the executor by name, which forced the running thread to continue in the same context.

thebrianchen · 2021-02-04T23:49:24Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java

+   * Sends the current batch and resets {@link #bulkCommitBatch}.
+   *
+   * @param flush If provided, keeps re-sending operations until no more operations are enqueued.
+   *     This allows retries to resolve as part of a {#link flush()} or {#link close()} call.


s/#link/@link

thebrianchen · 2021-02-04T23:53:53Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java

-      } else {
-        bulkWriterExecutor.schedule(
-            new Runnable() {
+  public void sendOperation(


This can be private.

thebrianchen · 2021-02-05T00:00:07Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOperation.java

+  private final Executor errorExecutor;
+  private final BulkWriter.WriteErrorCallback errorListener;
+
+  private int failedAttempts;


possible nit: Do we initialize variables in java even if it's implicit?

I couldn't find anything in the style guide, but it's a tad easier to read if we initialize here.

thebrianchen · 2021-02-05T00:07:52Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOperation.java

+ * Represents a single write for BulkWriter, encapsulating operation dispatch and error handling.
+ */
+class BulkWriterOperation {
+  private final SettableApiFuture<WriteResult> future = SettableApiFuture.create();


optional: rename to operationFuture to avoid confusion with the local future variables in the error and success handlers (or maybe it's just me that gets confused).

Done. Also renamed the other Futures to callbackFuture.

thebrianchen · 2021-02-05T00:10:08Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOperation.java

+        new ApiFutureCallback<Boolean>() {
+          @Override
+          public void onFailure(Throwable throwable) {
+            BulkWriterOperation.this.future.setException(throwable);


If you rename the instance future variable, can you remove the need to use BulkWriterOperation.this?

Yes, cleaned up.

thebrianchen · 2021-02-05T00:12:30Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/BulkWriterTest.java

+              GrpcStatusCode.of(Status.Code.ABORTED),
+              true));
+
+  // @Rule public Timeout timeout = new Timeout(500, TimeUnit.MILLISECONDS);


Can't have BulkWriter mysteriously flaking everywhere again.

This Rule made debugging so difficult. I kept commenting it in and out. But the end state should be "commented in" :)

thebrianchen · 2021-02-05T00:14:03Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/BulkWriterTest.java

 public class BulkWriterTest {

-  @Rule public Timeout timeout = new Timeout(500, TimeUnit.MILLISECONDS);
+  private static final ApiFuture<GeneratedMessageV3> FAILED_FUTURE =


This is a very sleek cleanup.

thebrianchen · 2021-02-05T00:17:02Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkCommitBatch.java

-import javax.annotation.Nullable;
+import java.util.concurrent.Executor;

 /** Used to represent a batch on the BatchQueue. */


Update class name?
My attempt: A batch that holds and schedules BulkWriter operations.

Hm. This should still match the original description. The pendingOperations are the operations that are scheduled on this batch. They might be scheduled on another batch too, but the scheduling itself doesn't take part here. Let me know if I am confused :)

The main issue I had was that "BatchQueue" is no longer a term used in BulkWriter.

thebrianchen · 2021-02-05T00:24:51Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkCommitBatch.java

-  boolean isReadyToSend() {
-    return state == BatchState.READY_TO_SEND;
+  void enqueueOperation(BulkWriterOperation operation) {
+    boolean added = documents.add(operation.getDocumentReference());


Wow, that optimization to add then check 🤯

This will save so much CPU that we can soon start mining Bitcoins in the SDK.

schmidt-sebastian

Thanks for pushing me to fix the flakes. I think this uncovered an actual issue where the batch was sometimes modified by multiple threads (through the backoff handler as well as the user input).

schmidt-sebastian · 2021-02-05T16:52:19Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkCommitBatch.java

-import javax.annotation.Nullable;
+import java.util.concurrent.Executor;

 /** Used to represent a batch on the BatchQueue. */


Hm. This should still match the original description. The pendingOperations are the operations that are scheduled on this batch. They might be scheduled on another batch too, but the scheduling itself doesn't take part here. Let me know if I am confused :)

schmidt-sebastian · 2021-02-05T16:52:49Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkCommitBatch.java

-  boolean isReadyToSend() {
-    return state == BatchState.READY_TO_SEND;
+  void enqueueOperation(BulkWriterOperation operation) {
+    boolean added = documents.add(operation.getDocumentReference());


This will save so much CPU that we can soon start mining Bitcoins in the SDK.

schmidt-sebastian · 2021-02-05T16:56:00Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java

+   * Sends the current batch and resets {@link #bulkCommitBatch}.
+   *
+   * @param flush If provided, keeps re-sending operations until no more operations are enqueued.
+   *     This allows retries to resolve as part of a {#link flush()} or {#link close()} call.


schmidt-sebastian · 2021-02-05T16:56:12Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java

-      } else {
-        bulkWriterExecutor.schedule(
-            new Runnable() {
+  public void sendOperation(


schmidt-sebastian · 2021-02-05T16:58:46Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOperation.java

+  private final Executor errorExecutor;
+  private final BulkWriter.WriteErrorCallback errorListener;
+
+  private int failedAttempts;


I couldn't find anything in the style guide, but it's a tad easier to read if we initialize here.

schmidt-sebastian · 2021-02-05T17:02:43Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOperation.java

+ * Represents a single write for BulkWriter, encapsulating operation dispatch and error handling.
+ */
+class BulkWriterOperation {
+  private final SettableApiFuture<WriteResult> future = SettableApiFuture.create();


Done. Also renamed the other Futures to callbackFuture.

schmidt-sebastian · 2021-02-05T17:02:52Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOperation.java

+        new ApiFutureCallback<Boolean>() {
+          @Override
+          public void onFailure(Throwable throwable) {
+            BulkWriterOperation.this.future.setException(throwable);


Yes, cleaned up.

schmidt-sebastian · 2021-02-05T17:03:41Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/BulkWriterTest.java

+              GrpcStatusCode.of(Status.Code.ABORTED),
+              true));
+
+  // @Rule public Timeout timeout = new Timeout(500, TimeUnit.MILLISECONDS);


This Rule made debugging so difficult. I kept commenting it in and out. But the end state should be "commented in" :)

thebrianchen

Thanks for diving into this one, really appreciate it :)

…java-firestore into mrschmidt/simplifyBulkWriter

product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Feb 3, 2021

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

chore: simplify BulkWriter

5ac45bf

schmidt-sebastian force-pushed the mrschmidt/simplifyBulkWriter branch from eabb8f3 to 5ac45bf Compare February 3, 2021 19:05

schmidt-sebastian changed the title ~~Mrschmidt/simplify bulk writer~~ chore: simplify BulkWriter Feb 3, 2021

schmidt-sebastian marked this pull request as ready for review February 3, 2021 19:09

schmidt-sebastian requested a review from a team as a code owner February 3, 2021 19:09

schmidt-sebastian requested review from a team and thebrianchen February 3, 2021 19:09

schmidt-sebastian assigned thebrianchen Feb 3, 2021

Add Copyright

0ab1e48

thebrianchen suggested changes Feb 5, 2021

View reviewed changes

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Feb 5, 2021

Fix multi-threaded access

cba1861

schmidt-sebastian commented Feb 5, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Feb 5, 2021

format

dc34324

thebrianchen approved these changes Feb 5, 2021

View reviewed changes

schmidt-sebastian added 2 commits February 5, 2021 14:36

Update BulkCommitBatch.java

c881307

Update BulkWriter.java

7b3dd7f

schmidt-sebastian added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. labels Feb 5, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 5, 2021

schmidt-sebastian added 4 commits February 5, 2021 15:09

Bump timeout to 1s

45c21bd

Merge branch 'mrschmidt/simplifyBulkWriter' of github.com:googleapis/…

b85e95e

…java-firestore into mrschmidt/simplifyBulkWriter

Fix flushCompletesWhenAllWritesComplete

ea833d6

Deflake

ee7a945

gcf-merge-on-green bot merged commit 3ed79ae into master Feb 5, 2021

gcf-merge-on-green bot deleted the mrschmidt/simplifyBulkWriter branch February 5, 2021 22:54

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 5, 2021

chore: simplify BulkWriter #523

chore: simplify BulkWriter #523

Uh oh!

Conversation

schmidt-sebastian commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

thebrianchen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

thebrianchen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

schmidt-sebastian commented Feb 3, 2021 •

edited

Loading

codecov bot commented Feb 3, 2021 •

edited

Loading

thebrianchen left a comment •

edited

Loading