fix: bulkWriter: writing to the same doc doesn't create a new batch by thebrianchen · Pull Request #394 · googleapis/java-firestore

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

 @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

- })
- .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

 @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


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


s/getKey/getDocumentKey

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

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


s/key/documentKey (or documentPath)

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

+ 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

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


This code almost looks like the future!

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

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


Suggested change
pendingOperations.add(
newPendingOperation(getPendingOperationCount(), documentReference.getPath(), result));
intnextBatchIndex = getPendingOperationCount();
pendingOperations.add(
newPendingOperation(nextBatchIndex, documentReference.getPath(), result));
Optional, but it does remove the need to understand the execution order.

thebrianchen self-assigned this Oct 1, 2020

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

product-auto-labelBot 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 c9a4853Compare October 1, 2020 16:24

fix clirr rules
73af088

thebrianchen requested review from a team and schmidt-sebastian October 1, 2020 19:15

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 masterOct 1, 2020

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

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

-pendingOperations.add(
-newPendingOperation(getPendingOperationCount(), documentReference.getPath(), result));
+intnextBatchIndex = getPendingOperationCount();
+pendingOperations.add(
+newPendingOperation(nextBatchIndex, documentReference.getPath(), result));

Uh oh!

Conversation

thebrianchen commented Oct 1, 2020

Uh oh!

codecovBot commented Oct 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecovBot commented Oct 1, 2020•
edited
Loading