feat: add success and error callbacks to BulkWriter #483

thebrianchen · 2020-12-10T18:10:36Z

Continuation of #458 but without blocking gax/grpc executor!

…ion limits

* Add new userCallbackExecutor which is used to ensure user callbacks aren't able to block any gax thread. Currently this is creating a cached thread pool per writer, we should likely treat this thread pool as a singleton and pass it from the builder to leverage the reuse of the threads even past the lifetime of the writer. * Update instances of setting a SettableFuture to only set after the state operation (add/remove) has completed to ensure and downstream future will only start after state update.

…firestore into bc/bulk-error-with-test

schmidt-sebastian

Very nice. I am happy that we managed to get this to work!

schmidt-sebastian · 2020-12-11T16:01:07Z

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

+    ApiFuture<BatchWriteResponse> response =
+        firestore.sendRequest(request.build(), firestore.getClient().batchWriteCallable());
+
+    committed = true;


Just noticed that this is missing from Node.

Would it make sense to make this an abstract property?

In UpdateBuilder, you could do:

boolean isCommitted() { return committed; }

and here you could do:

boolean isCommitted() { return batchState == SENT; }

Done. I'll back-port this to node later along with some of the other changes like the retry logging message.

schmidt-sebastian · 2020-12-11T16:06:53Z

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

@@ -42,9 +44,44 @@
 import javax.annotation.Nullable;

 final class BulkWriter implements AutoCloseable {
+  /**
+   * A callback set by `addWriteResultListener()` to be run every time an operation successfully


Maybe s/set/used ? I am not sure though.

I think it should be "set" since the addWriteResultListener() function sets the callback, which is then run on operation success.

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

schmidt-sebastian · 2020-12-11T16:10:23Z

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

+   * modification errors (as this list is modified from both the user thread and the network
+   * thread).
+   */
+  private final List<BulkCommitBatch> retryBatchQueue = new CopyOnWriteArrayList<>();


Would it be possible to schedule everything on bulkWriterExecutor now and use a normal ArrayList? The enqueue part is already on the executor, so the rest should be either be on it as well or be relatively easy to move.

Good point, done.

schmidt-sebastian · 2020-12-11T16:11:55Z

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

+
+  /**
+   * A set of futures that represent pending BulkWriter operations. Each future is completed when
+   * the BulkWriter operation resolves. This set includes retries. Each retry's promise is added,


"This set includes retries" is not very specific. Do we:

Add a new operation for each retry (looks like we don't)

Or do we re-use the existing operations?

Updated the documentation and removed the incorrect parts.

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

schmidt-sebastian · 2020-12-11T17:49:17Z

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

+
+    // Verify that the 2nd operation did not complete as a result of the flush() call.
+    assertArrayEquals(new String[] {"BEFORE_FLUSH", "FLUSH"}, operations.toArray());
+    bulkWriter.close();


It might make sense to add another flush here and verify "AFTER_FLUSH" as well. That way, it doesn't look unused.

schmidt-sebastian · 2020-12-11T17:52:00Z

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

@@ -422,8 +743,8 @@ public void retriesIndividualWritesThatFailWithAbortedOrUnavailable() throws Exc
      result1.get();
      fail("set() should have failed");
    } catch (Exception e) {
-      assertTrue(e.getCause() instanceof FirestoreException);
-      assertEquals(Status.DEADLINE_EXCEEDED, ((FirestoreException) e.getCause()).getStatus());
+      assertTrue(e.getCause() instanceof BulkWriterError);


Could BulkWriterError extend FirestoreException? That way, it can be commonly treated by our users.

BulkWriterError should likely also be BulkWriterException. See the first part of the first answer here: https://stackoverflow.com/questions/5813614/what-is-difference-between-errors-and-exceptions

I attempted it. I had to make FirestoreException not final in order to extend, and I had to make one of the private constructors package-private.

codecov · 2020-12-12T00:46:43Z

Codecov Report

Merging #483 (c1264e0) into master (a979c02) will increase coverage by 1.11%.
The diff coverage is 96.27%.

@@             Coverage Diff              @@
##             master     #483      +/-   ##
============================================
+ Coverage     73.33%   74.45%   +1.11%     
- Complexity     1095     1118      +23     
============================================
  Files            65       66       +1     
  Lines          5790     5852      +62     
  Branches        721      722       +1     
============================================
+ Hits           4246     4357     +111     
+ Misses         1317     1267      -50     
- Partials        227      228       +1

Impacted Files	Coverage Δ	Complexity Δ
...com/google/cloud/firestore/FirestoreException.java	`83.33% <ø> (ø)`	`9.00 <0.00> (ø)`
...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> (ø)`
...java/com/google/cloud/firestore/UpdateBuilder.java	`94.97% <88.88%> (-1.20%)`	`59.00 <3.00> (-19.00)`
...om/google/cloud/firestore/BulkWriterException.java	`91.66% <91.66%> (ø)`	`5.00 <5.00> (?)`
...va/com/google/cloud/firestore/BulkCommitBatch.java	`93.05% <92.75%> (+2.14%)`	`18.00 <16.00> (+14.00)`
...in/java/com/google/cloud/firestore/BulkWriter.java	`98.23% <98.03%> (+26.12%)`	`54.00 <15.00> (+23.00)`
...java/com/google/cloud/firestore/FirestoreImpl.java	`76.08% <100.00%> (+0.17%)`	`27.00 <1.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 a979c02...c1264e0. Read the comment docs.

… wasting logic

schmidt-sebastian

LGTM. Thanks for not giving up!

schmidt-sebastian · 2020-12-14T22:07:56Z

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

+  /**
+   * A queue of batches to be retried. Use a synchronized list to avoid multi-thread concurrent
+   * modification errors (as this list is modified from both the user thread and the network
+   * thread).


Comment is outdated.

BenWhitehead · 2020-12-15T19:44:09Z

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

+    void onResult(DocumentReference documentReference, WriteResult result);
+  };
+
+  /** A callback set by `addWriteErrorListener()` to be run every time an operation fails. */


... "And determines if an operation should be retried"

BenWhitehead · 2020-12-15T19:48:32Z

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


  /** Whether this BulkWriter instance is closed. Once closed, it cannot be opened again. */
  private boolean closed = false;

  /** Rate limiter used to throttle requests as per the 500/50/5 rule. */
  private final RateLimiter rateLimiter;

-  private final FirestoreImpl firestore;
+  private WriteResultCallback successListener =


The value of this can be turned into a static final constant and then referenced as a default so there is only one instance per JVM, rather than an instance per BulkWriter.

BenWhitehead · 2020-12-15T19:48:47Z

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

+        public void onResult(DocumentReference documentReference, WriteResult result) {}
+      };
+
+  private WriteErrorCallback errorListener =


Same note as successListener

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

BenWhitehead · 2020-12-15T20:08:40Z

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

    return flushComplete;
  }

  /**
   * Commits all enqueued writes and marks the BulkWriter instance as closed.
   *
-   * <p>After calling `close()`, calling any method wil return an error.
+   * <p>After calling `close()`, calling any method wil return an error. Any retries scheduled with


Suggested change

* <p>After calling `close()`, calling any method wil return an error. Any retries scheduled with

* <p>After calling `close()`, calling any method will return an error. Any retries scheduled with

good catch, thanks

BenWhitehead · 2020-12-15T20:10:07Z

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

    return flushComplete;
  }

  /**
   * Commits all enqueued writes and marks the BulkWriter instance as closed.
   *
-   * <p>After calling `close()`, calling any method wil return an error.
+   * <p>After calling `close()`, calling any method wil return an error. Any retries scheduled with
+   * `addWriteErrorListener()` will be run before the `close()` completes.


Is the same true for success listeners? If so let's document it.

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

BenWhitehead · 2020-12-15T20:22:33Z

google-cloud-firestore/clirr-ignored-differences.xml

+  <difference>
+    <differenceType>7002</differenceType>
+    <className>com/google/cloud/firestore/UpdateBuilder</className>
+    <method>int getMutationsSize()</method>


We can't remove this method without a major version release since it's public. because it's public customers could be using already and we won't want to remove it and break them.

We should be able to keep the method public without any issues.

Changed back to public.

BenWhitehead · 2020-12-15T20:23:48Z

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

-    }
+  /** Get the number of writes. */
+  @VisibleForTesting
+  int getWriteCount() {


We need to keep this method public for binary compatibility as I noted in the clirr file.

After making this public we no longer need the @VisibleForTesting.

Brian Chen and others added 14 commits November 6, 2020 23:05

chore: refactor BulkWriter to use retryBatch

f8e53e7

fix: create new batch for writes to the same doc

f4d53b4

refactor BulkWriter's logic in UpdateBuilder into BulkCommitBatch

9673dfa

set MAX_BATCH_SIZE to 20 now that it won't affect WriteBatch/Transact…

d92d1da

…ion limits

cleanup

3d895e1

add clirr exceptions

906165a

address rd. 1 comments

2b5f04a

change naming to onError and expose callback executors

2133403

add system tests

4ed2e47

add todo

9a75859

Merge branch 'bc/bulk-error-with-test' of github.com:googleapis/java-…

1ece19e

…firestore into bc/bulk-error-with-test

add slf4j logging to help with debugging

1b63018

Move all BulkWriter logic to its own thread + cleanup

49fe275

thebrianchen requested a review from a team as a code owner December 10, 2020 18:10

thebrianchen self-assigned this Dec 10, 2020

thebrianchen requested a review from a team December 10, 2020 18:10

thebrianchen requested a review from a team as a code owner December 10, 2020 18:10

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

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

Brian Chen added 2 commits December 10, 2020 10:32

Merge branch 'master' into bc/bulk-error-with-test

a5b372b

remove usage of defaultThreadFactory

028e373

thebrianchen requested a review from schmidt-sebastian December 10, 2020 20:20

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

schmidt-sebastian reviewed Dec 11, 2020

View reviewed changes

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

Brian Chen added 2 commits December 11, 2020 15:34

resolve comments rd.1

95fdcde

remove userCallbackExecutor and add clirr exception

b984f56

fix flaky test

59c89bc

fix flaking batch limiter test

c46a296

thebrianchen requested a review from schmidt-sebastian December 14, 2020 17:59

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

thebrianchen mentioned this pull request Dec 14, 2020

feat: add error and success handlers to BulkWriter #458

Closed

use directExecutor() in success/error executors by default instead of…

044f925

… wasting logic

schmidt-sebastian approved these changes Dec 14, 2020

View reviewed changes

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

update comment since we're not using synchronized lists anymore

8dc465d

BenWhitehead requested changes Dec 15, 2020

View reviewed changes

Brian Chen added 2 commits December 17, 2020 15:27

resolve ben comments, add coverage

bf03904

missed a codecov test

c1264e0

thebrianchen requested a review from BenWhitehead December 17, 2020 23:47

thebrianchen assigned BenWhitehead and unassigned thebrianchen Dec 17, 2020

BenWhitehead approved these changes Jan 7, 2021

View reviewed changes

thebrianchen merged commit 3c05037 into master Jan 14, 2021

thebrianchen deleted the bc/bulk-error-with-test branch January 14, 2021 00:43

thebrianchen pushed a commit that referenced this pull request Jan 14, 2021

feat: add success and error callbacks to BulkWriter (#483)

04f0ab7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add success and error callbacks to BulkWriter #483

feat: add success and error callbacks to BulkWriter #483

thebrianchen commented Dec 10, 2020

schmidt-sebastian left a comment

schmidt-sebastian Dec 11, 2020

thebrianchen Dec 11, 2020

schmidt-sebastian Dec 11, 2020

thebrianchen Dec 11, 2020

schmidt-sebastian Dec 11, 2020

thebrianchen Dec 11, 2020

schmidt-sebastian Dec 11, 2020

thebrianchen Dec 11, 2020

schmidt-sebastian Dec 11, 2020

thebrianchen Dec 11, 2020

schmidt-sebastian Dec 11, 2020

thebrianchen Dec 11, 2020

codecov bot commented Dec 12, 2020 •

edited

schmidt-sebastian left a comment

schmidt-sebastian Dec 14, 2020

thebrianchen Dec 14, 2020

BenWhitehead Dec 15, 2020

thebrianchen Dec 16, 2020

BenWhitehead Dec 15, 2020

thebrianchen Dec 16, 2020

BenWhitehead Dec 15, 2020

thebrianchen Dec 16, 2020

BenWhitehead Dec 15, 2020

thebrianchen Dec 16, 2020

BenWhitehead Dec 15, 2020

thebrianchen Dec 16, 2020

BenWhitehead Dec 15, 2020

thebrianchen Dec 16, 2020

BenWhitehead Dec 15, 2020

thebrianchen Dec 16, 2020

	* <p>After calling `close()`, calling any method wil return an error. Any retries scheduled with
	* <p>After calling `close()`, calling any method will return an error. Any retries scheduled with

feat: add success and error callbacks to BulkWriter #483

feat: add success and error callbacks to BulkWriter #483

Conversation

thebrianchen commented Dec 10, 2020

schmidt-sebastian left a 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 Dec 12, 2020 • edited

Codecov Report

schmidt-sebastian left a 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 Dec 12, 2020 •

edited