feat: add error and success handlers to BulkWriter #458

thebrianchen · 2020-11-09T20:14:30Z

Porting of several node PRs: 1, 2, 3.

Another fix I made was reducing the MAX_BATCH_SIZE from 500 to 20. I had to do this after refactoring UpdateBuilder, since the previous code imposed the same limits on WriteBatch/Transactions, causing the system tests to fail.

I've tried to order the commits to make it more reviewable, but after finishing it up the first time, I went back and did more cleanup.

…ion limits

thebrianchen · 2020-11-09T21:18:50Z

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

+   *
+   * <p>The writes in the batch are not applied atomically and can be applied out of order.
+   */
+  ApiFuture<List<BatchWriteResult>> bulkCommit() {


I tried extracting a commitHelper() from bulkCommit() and commit(), but couldn't figure out how to properly parameterize everything.

Aside from reducing code duplication, the main reason I want to do this is because moving this over requires a getter for writes and exposing committed as protected. If we could move the logic for these parts into UpdateBuilder, we could make both of those variables private.

I tried to come up with something here as well but mostly failed. The two Proto classes don't share a common ancestor that would help us here, which rules out extracting a common function that operates on a shared base type.
My one suggestion would be to have a prepareCommit() method that returns a list of Writes and sets committed to true. I, however, don't think that this will actually save any code or make the code more readable. I am not sure if that is worth pursuing.

Ok, if that's the case I think we should just not try and refactor then.

codecov · 2020-11-09T21:24:29Z

Codecov Report

Merging #458 (2133403) into master (3893c44) will increase coverage by 0.31%.
The diff coverage is 84.92%.

@@             Coverage Diff              @@
##             master     #458      +/-   ##
============================================
+ Coverage     73.40%   73.71%   +0.31%     
- Complexity     1086     1091       +5     
============================================
  Files            65       66       +1     
  Lines          5786     5828      +42     
  Branches        723      724       +1     
============================================
+ Hits           4247     4296      +49     
+ Misses         1313     1306       -7     
  Partials        226      226

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> (ø)`
...in/java/com/google/cloud/firestore/BulkWriter.java	`82.39% <80.43%> (+9.80%)`	`39.00 <9.00> (+8.00)`
...va/com/google/cloud/firestore/BulkWriterError.java	`91.66% <91.66%> (ø)`	`5.00 <5.00> (?)`
...va/com/google/cloud/firestore/BulkCommitBatch.java	`94.44% <94.20%> (+3.53%)`	`17.00 <15.00> (+13.00)`
...java/com/google/cloud/firestore/UpdateBuilder.java	`94.92% <100.00%> (-1.25%)`	`58.00 <3.00> (-20.00)`
...java/com/google/cloud/firestore/FirestoreImpl.java	`77.61% <0.00%> (ø)`	`26.00% <0.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 3893c44...4c9e41a. Read the comment docs.

schmidt-sebastian

This looks really good!

schmidt-sebastian · 2020-11-10T17:40:25Z

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

+   *
+   * <p>The writes in the batch are not applied atomically and can be applied out of order.
+   */
+  ApiFuture<List<BatchWriteResult>> bulkCommit() {


I tried to come up with something here as well but mostly failed. The two Proto classes don't share a common ancestor that would help us here, which rules out extracting a common function that operates on a shared base type.
My one suggestion would be to have a prepareCommit() method that returns a list of Writes and sets committed to true. I, however, don't think that this will actually save any code or make the code more readable. I am not sure if that is worth pursuing.

schmidt-sebastian · 2020-11-10T17:57:45Z

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

+   * Resolves the individual operations in the batch with the results and removes the entry from the
+   * pendingOperations map if the result is not retryable.
+   */
+  void newProcessResults(List<BatchWriteResult> results) {


Is this the final name for this function? :)

oops, changed.

schmidt-sebastian · 2020-11-10T17:59:13Z

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

+     * @param error The error object from the failed BulkWriter operation attempt.
+     * @return Whether to retry the operation or not.
+     */
+    boolean shouldRetryListener(BulkWriterError error);


This is not a listener, this is the function that is invoked by the listener. I think this should be onError, shouldRetry, or shouldRetryOnError.

schmidt-sebastian · 2020-11-10T18:10:13Z

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

+                    operationType,
+                    failedAttempts);
+
+            boolean shouldRetry = errorListener.shouldRetryListener(bulkWriterError);


This should be executed on the User-specified executor (via FirestoreOptions - the same as

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

Line 87 in 5905438

this.userCallbackExecutor =

)

Done here and in the success callback.

schmidt-sebastian · 2020-11-10T18:16:23Z

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

+    return ApiFutures.transformAsync(
+        ApiFutures.catchingAsync(
+            batch.bulkCommit(),
+            Exception.class,


This should probably be a Throwable so we can also catch RuntimeException.

Based on Java docs, RunTimeException extends Exception. The main issue with using Throwable is that BatchWriteResult takes in Exception, and I would have to plumb that through.

schmidt-sebastian · 2020-11-10T18:19:28Z

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

+    bulkWriter.addWriteErrorListener(
+        new WriteErrorCallback() {
+          public boolean shouldRetryListener(BulkWriterError error) {
+            errorListenerCalled[0] = true;


Maybe assert that we received an INTERNAL error here.

schmidt-sebastian · 2020-11-10T18:21:01Z

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

+    bulkWriter.addWriteErrorListener(
+        new WriteErrorCallback() {
+          public boolean shouldRetryListener(BulkWriterError error) {
+            throw new NullPointerException("Test code threw NullPointerException");


This is probably not a good exception type since this is likely thrown by our code as well. I would advise to use something different - maybe UnsupportedOperationException?

schmidt-sebastian · 2020-11-10T18:21:16Z

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

+    bulkWriter.addWriteResultListener(
+        new WriteResultCallback() {
+          public void onResult(DocumentReference documentReference, WriteResult result) {
+            throw new NullPointerException("Test code threw NullPointerException");


Same comment as above.

schmidt-sebastian · 2020-11-10T20:06:12Z

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

+   *   bulkWriter.addWriteResultListener(
+   *         new WriteResultCallback() {
+   *           public void onResult(DocumentReference documentReference, WriteResult result) {
+   *             System.out.println(
+   *                 "Successfully executed write on document: "
+   *                     + documentReference
+   *                     + " at: "
+   *                     + result.getUpdateTime());
+   *           }
+   *         });


You should probably just use Lamdba syntax here.

schmidt-sebastian · 2020-11-10T20:06:51Z

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

+   *               System.out.println("Failed write at document: " + error.getDocumentReference());
+   *               return false;
+   *             }
+   *         });


You should probably use Lambda syntax here. If you stick with the current example, you need to add the function signature.

schmidt-sebastian · 2020-11-10T23:34:24Z

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

@@ -59,13 +61,15 @@
     * @param error The error object from the failed BulkWriter operation attempt.
     * @return Whether to retry the operation or not.
     */
-    boolean shouldRetryListener(BulkWriterError error);
+    boolean shouldRetryError(BulkWriterError error);


I am thinking more and more that this should be onError. Otherwise, users are going to ask us how they can get the error result and why we don't expose it in WriteResultCallback. Note also that the term result is a bit ambiguous as it doesn't define whether "error" is a "result".

We should maybe also ask for a third opinion of whether the classes are "callbacks". On Android, we seem to call the outermost class "Listener". The method inside is prefixed with "on" (see EventListener).

Changed to onError. As for WriteResult, we discussed naming for the node implementation and decided on WriteResult over WriteSuccess since a WriteResult is returned from the operation.

Would it be too sadistic to ask Ben to take a look at this PR for Java code style/convention aftewards?

We should definitely ask Ben :)

schmidt-sebastian · 2020-11-10T23:43:39Z

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

-                successListener.onResult(documentReference, result);
+              public ApiFuture<WriteResult> apply(WriteResult result)
+                  throws ExecutionException, InterruptedException {
+                invokeUserSuccessCallback(documentReference, result).get();


This looks like it may block the GRPC thread while the user code is running. Instead, you should do one of the following:

Run your code

Run user code in user thread, followed by a continuation that dispatches back on your old exectur

Finish running your own code

Realistically, though, no one is going to get mad if you run a little bit of your own code on the use executor (as long as it is non-blocking and doesn't throw). For that reason, you can likely just use the user executor when the callback is first invoked (e.g. replace the directExecutor call in executeWrite() if deemed safe).

The main issue with dispatching the code onto the user executor is that we're no longer guaranteed that the write is enqueued onto the BatchQueue before flush() is called. Instead, after our discussion, I exposed two user executors that the user could pass in to run their callbacks on. We're blocking on the user-callbacks, but not on the user or grpc threads.

Summary of conversation w/ Ben: We should not block grpc or gax threads. Instead, we should be creating a thread on each firestore instance that awaits user callbacks.

Solution: Created a separate thread for waiting on the callback. Fixed async issue by fixing bug in performFlush(), where we were incorrectly resolving the flushComplete future early if no retries were scheduled.

schmidt-sebastian · 2020-11-10T23:44:31Z

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

@@ -762,8 +806,7 @@ private void verifyNotClosed() {
   * <p>For example, see the sample code: <code>
   *   BulkWriter bulkWriter = firestore.bulkWriter();
   *   bulkWriter.addWriteResultListener(
-   *         new WriteResultCallback() {
-   *           public void onResult(DocumentReference documentReference, WriteResult result) {
+   *         (DocumentReference documentReference, WriteResult result -> {


Missing ")".

We should make sure these compile as users are going to copy them.

Added. Checked that code compiles.

…into bc/bulk-error

thebrianchen · 2020-12-14T18:03:37Z

Continued in #483.

Brian Chen added 3 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

thebrianchen requested a review from a team as a code owner November 9, 2020 20:14

thebrianchen requested a review from a team November 9, 2020 20:14

thebrianchen self-assigned this Nov 9, 2020

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

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

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

d92d1da

…ion limits

thebrianchen force-pushed the bc/bulk-error branch from 843cd2e to d92d1da Compare November 9, 2020 20:36

Brian Chen added 2 commits November 9, 2020 15:05

cleanup

3d895e1

add clirr exceptions

906165a

thebrianchen commented Nov 9, 2020

View reviewed changes

thebrianchen force-pushed the bc/bulk-error branch from dceab85 to 906165a Compare November 9, 2020 21:19

thebrianchen requested a review from a team as a code owner November 9, 2020 21:19

thebrianchen requested a review from schmidt-sebastian November 9, 2020 21:41

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Nov 9, 2020

schmidt-sebastian reviewed Nov 10, 2020

View reviewed changes

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

address rd. 1 comments

2b5f04a

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

thebrianchen requested a review from schmidt-sebastian November 10, 2020 22:53

schmidt-sebastian reviewed Nov 10, 2020

View reviewed changes

schmidt-sebastian removed their assignment Nov 10, 2020

change naming to onError and expose callback executors

2133403

thebrianchen requested a review from schmidt-sebastian November 11, 2020 22:26

thebrianchen assigned schmidt-sebastian Nov 11, 2020

Brian Chen added 5 commits November 12, 2020 19:07

add system tests

4ed2e47

add back timeout rule to test

1f7b0fc

add todo

9a75859

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

e267432

…into bc/bulk-error

remove bulkwriter integration test and make bulkWriter private

4c9e41a

thebrianchen mentioned this pull request Dec 10, 2020

feat: add success and error callbacks to BulkWriter #483

Merged

thebrianchen closed this Dec 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add error and success handlers to BulkWriter #458

feat: add error and success handlers to BulkWriter #458

thebrianchen commented Nov 9, 2020 •

edited

thebrianchen Nov 9, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 10, 2020

codecov bot commented Nov 9, 2020 •

edited

schmidt-sebastian left a comment

schmidt-sebastian Nov 10, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 10, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 10, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 10, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 10, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 10, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 10, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 10, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 10, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 10, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 11, 2020

schmidt-sebastian Nov 11, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 11, 2020 •

edited

thebrianchen Nov 13, 2020

schmidt-sebastian Nov 10, 2020

thebrianchen Nov 11, 2020

thebrianchen commented Dec 14, 2020

feat: add error and success handlers to BulkWriter #458

feat: add error and success handlers to BulkWriter #458

Conversation

thebrianchen commented Nov 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 9, 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

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

thebrianchen Nov 11, 2020 • edited

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

thebrianchen commented Dec 14, 2020

thebrianchen commented Nov 9, 2020 •

edited

codecov bot commented Nov 9, 2020 •

edited

thebrianchen Nov 11, 2020 •

edited