fix: bulkWriter: writing to the same doc doesn't create a new batch #394

thebrianchen · 2020-10-01T04:19:19Z

Porting from node.

codecov · 2020-10-01T04:25:20Z

Codecov Report

Merging #394 into master will decrease coverage by 0.09%.
The diff coverage is 82.25%.

@@             Coverage Diff              @@
##             master     #394      +/-   ##
============================================
- Coverage     72.49%   72.40%   -0.10%     
+ Complexity     1056     1043      -13     
============================================
  Files            64       64              
  Lines          5589     5578      -11     
  Branches        692      689       -3     
============================================
- Hits           4052     4039      -13     
- Misses         1320     1323       +3     
+ Partials        217      216       -1

Impacted Files	Coverage Δ	Complexity Δ
...in/java/com/google/cloud/firestore/BulkWriter.java	`67.00% <52.17%> (-4.03%)`	`22.00 <0.00> (-6.00)`
...a/com/google/cloud/firestore/BatchWriteResult.java	`100.00% <100.00%> (ø)`	`3.00 <1.00> (-1.00)`
...va/com/google/cloud/firestore/BulkCommitBatch.java	`90.90% <100.00%> (-2.43%)`	`4.00 <0.00> (ø)`
...java/com/google/cloud/firestore/UpdateBuilder.java	`96.16% <100.00%> (+0.53%)`	`78.00 <8.00> (-5.00)`	⬆️
...java/com/google/cloud/firestore/FirestoreImpl.java	`76.86% <0.00%> (ø)`	`25.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 5e8c154...3a2b8c6. Read the comment docs.

schmidt-sebastian

Basically LGTM. Some naming nits.

schmidt-sebastian · 2020-10-01T21:05:19Z

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

@@ -26,21 +26,18 @@
 */
 @InternalApi
 public final class BatchWriteResult {
-  private final DocumentReference documentReference;
+  private final String key;


It looks like BatchWriteResult could be package-private, which would avoid the CLIRR violation.

good point, done.

It added another CLIRR violation for changing to package private, but I changed the rule.

schmidt-sebastian · 2020-10-01T21:09:18Z

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

-                })
-            .toList());
+
+    // Create a new BulkCommitBatch containing only the indexes from the provided indexes to retry.


Hm. This is different from Node, but likely better. Should we update the Node SDK to match? This might make porting simpler.

Funny how having more tools at your disposal does not translate to writing better code. Will create a small node PR for that later.

schmidt-sebastian · 2020-10-01T21:11:00Z

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

@@ -26,21 +26,18 @@
 */
 @InternalApi
 public final class BatchWriteResult {
-  private final DocumentReference documentReference;
+  private final String key;


Also s/key/documentKey since this is not apparent from the classes context (or documentPath - see below).

Thanks to IntelliJ, realized this field is no longer needed. Will remove in Node as well.

schmidt-sebastian · 2020-10-01T21:11:36Z

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

    this.writeTime = timestamp;
    this.exception = exception;
  }

-  public DocumentReference getDocumentReference() {
-    return documentReference;
+  public String getKey() {


s/getKey/getDocumentKey

schmidt-sebastian · 2020-10-01T21:14:57Z

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

+   */
+  static class PendingOperation {
+    int writeBatchIndex;
+    String key;


s/key/documentKey (or documentPath)

schmidt-sebastian · 2020-10-01T21:18:07Z

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

+            new Function<PendingOperation, String>() {
+              @Override
+              public String apply(PendingOperation input) {
+                return input.key;


You should settle on either "key" or "path". I can see an argument for key (in that our DocumentReference.getPath method is relative to the root), but we usually use path to represent the full document path in its Proto representation. I think "key" is exactly that here.

I prefer documentKey, for the reasons you mentioned, and b/c it matches better with key from Node.

schmidt-sebastian · 2020-10-01T21:19:04Z

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

+                return input.future;
+              }
+            })
+        .toList();


This code almost looks like the future!

schmidt-sebastian · 2020-10-01T21:23:40Z

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

+    pendingOperations.add(
+        new PendingOperation(getPendingOperationCount(), documentReference.getPath(), result));


Suggested change

pendingOperations.add(

new PendingOperation(getPendingOperationCount(), documentReference.getPath(), result));

int nextBatchIndex = getPendingOperationCount();

pendingOperations.add(

new PendingOperation(nextBatchIndex, documentReference.getPath(), result));

Optional, but it does remove the need to understand the execution order.

thebrianchen self-assigned this Oct 1, 2020

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

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

fix: bulkWriter: writing to the same doc doesn't create a new batch

c9a4853

thebrianchen force-pushed the bc/bulk-no-order branch from f7ed61a to c9a4853 Compare October 1, 2020 16:24

fix clirr rules

73af088

thebrianchen requested a review from a team as a code owner October 1, 2020 19:15

thebrianchen requested a review from schmidt-sebastian October 1, 2020 19:23

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Oct 1, 2020

schmidt-sebastian reviewed Oct 1, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Oct 1, 2020

Brian Chen added 3 commits October 1, 2020 16:49

resolve comments

e5f1388

remove usage of BatchWriteResult.documentKey

d0cbc32

clirr rule update

3a2b8c6

thebrianchen requested a review from schmidt-sebastian October 1, 2020 22:02

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Oct 1, 2020

schmidt-sebastian approved these changes Oct 1, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Oct 1, 2020

BenWhitehead approved these changes Oct 1, 2020

View reviewed changes

BenWhitehead merged commit 259ece8 into master Oct 1, 2020

BenWhitehead deleted the bc/bulk-no-order branch October 1, 2020 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bulkWriter: writing to the same doc doesn't create a new batch #394

fix: bulkWriter: writing to the same doc doesn't create a new batch #394

thebrianchen commented Oct 1, 2020

codecov bot commented Oct 1, 2020 •

edited

schmidt-sebastian left a comment

schmidt-sebastian Oct 1, 2020

thebrianchen Oct 1, 2020

thebrianchen Oct 1, 2020

schmidt-sebastian Oct 1, 2020

thebrianchen Oct 1, 2020

schmidt-sebastian Oct 1, 2020

thebrianchen Oct 1, 2020

schmidt-sebastian Oct 1, 2020

thebrianchen Oct 1, 2020

schmidt-sebastian Oct 1, 2020

thebrianchen Oct 1, 2020

schmidt-sebastian Oct 1, 2020

thebrianchen Oct 1, 2020

schmidt-sebastian Oct 1, 2020

schmidt-sebastian Oct 1, 2020

thebrianchen Oct 1, 2020

		pendingOperations.add(
		new PendingOperation(getPendingOperationCount(), documentReference.getPath(), result));

fix: bulkWriter: writing to the same doc doesn't create a new batch #394

fix: bulkWriter: writing to the same doc doesn't create a new batch #394

Conversation

thebrianchen commented Oct 1, 2020

codecov bot commented Oct 1, 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 Oct 1, 2020 •

edited