fix: add bulkCommit() and BatchWrite protos by thebrianchen · Pull Request #231 · googleapis/java-firestore

thebrianchen · 2020-05-28T19:21:38Z

Porting from node.

codecov · 2020-05-28T19:31:47Z

Codecov Report

Merging #231 into bc/bulk-master will increase coverage by 0.61%.
The diff coverage is 83.68%.

@@                 Coverage Diff                  @@
##             bc/bulk-master     #231      +/-   ##
====================================================
+ Coverage             73.01%   73.62%   +0.61%     
- Complexity             1024     1048      +24     
====================================================
  Files                    64       65       +1     
  Lines                  5443     5650     +207     
  Branches                620      622       +2     
====================================================
+ Hits                   3974     4160     +186     
- Misses                 1271     1296      +25     
+ Partials                198      194       -4

Impacted Files	Coverage Δ	Complexity Δ
...oogle/cloud/firestore/spi/v1/GrpcFirestoreRpc.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...gle/cloud/firestore/v1/FirestoreAdminSettings.java	`13.20% <0.00%> (-2.35%)`	`2.00 <0.00> (ø)`
...m/google/cloud/firestore/v1/FirestoreSettings.java	`16.36% <0.00%> (-0.62%)`	`4.00 <0.00> (ø)`
...le/cloud/firestore/v1/stub/FirestoreAdminStub.java	`5.88% <0.00%> (-2.46%)`	`1.00 <0.00> (ø)`
.../google/cloud/firestore/v1/stub/FirestoreStub.java	`5.88% <0.00%> (-0.37%)`	`1.00 <0.00> (ø)`
...gle/cloud/firestore/v1beta1/FirestoreSettings.java	`13.20% <0.00%> (ø)`	`2.00 <0.00> (ø)`
...le/cloud/firestore/v1beta1/stub/FirestoreStub.java	`6.25% <0.00%> (ø)`	`1.00 <0.00> (ø)`
...oogle/cloud/firestore/v1/FirestoreAdminClient.java	`57.46% <50.00%> (-2.38%)`	`38.00 <23.00> (ø)`
.../firestore/v1beta1/stub/FirestoreStubSettings.java	`82.35% <79.68%> (ø)`	`23.00 <5.00> (ø)`
...cloud/firestore/v1/stub/FirestoreStubSettings.java	`82.60% <90.00%> (+0.25%)`	`24.00 <1.00> (+1.00)`
... and 12 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 b078213...7363e48. Read the comment docs.

BenWhitehead

We'll need to wait on merging this until the protos come from synth and googleapis otherwise these new files will be removed. We can get it all fixed up and read to merge once they are available, but we don't want to merge this until then.

BenWhitehead · 2020-05-28T20:51:57Z

The main change adding the buildwriter stuff looks okay to me, but I'm leaving my "Requires changes" review status until we can get the proto issues resolved in master.

thebrianchen · 2020-05-28T21:22:24Z

@BenWhitehead I'm trying to merge this into a separate branch (not master) that I plan on using to port BulkWriter into. Is it feasible to develop on a separate branch and then try to merge into master once the proto issues are resolved?

BenWhitehead

This is okay to merge to the feature branch, will coordinate at a later time for the feature branches merge to master.

schmidt-sebastian

LGTM

schmidt-sebastian · 2020-05-29T04:48:49Z

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

+    this.status = status;
+  }
+
+  public Timestamp getWriteTime() {


Add @Nullable.

schmidt-sebastian · 2020-05-29T04:48:53Z

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

+ * BatchWriteRequests.
+ */
+public final class BatchWriteResult {
+  private final Timestamp writeTime;


Add @Nullable.

schmidt-sebastian · 2020-05-29T04:54:24Z

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

+  @Test
+  public void bulkCommit() throws Exception {
+    BatchWriteResponse.Builder response = BatchWriteResponse.newBuilder();
+    response.addWriteResultsBuilder().getUpdateTimeBuilder().setSeconds(0).setNanos(1);


This is technically missing one extra call to addWriteResultsBuilder() (with not update time).

You can also drop the call to setSeconds(0) since it is the default value.

schmidt-sebastian · 2020-05-29T04:55:22Z

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

+    BatchWriteResponse.Builder response = BatchWriteResponse.newBuilder();
+    response.addWriteResultsBuilder().getUpdateTimeBuilder().setSeconds(0).setNanos(1);
+    response.addWriteResultsBuilder();
+    response.addStatusBuilder().setCode(0).build();


setCode(0) is the default.

schmidt-sebastian · 2020-05-29T04:56:41Z

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

+    List<BatchWriteResult> batchWriteResults = batch.bulkCommit().get();
+
+    assertEquals(Timestamp.ofTimeSecondsAndNanos(0, 1), batchWriteResults.get(0).getWriteTime());
+    assertEquals(Status.OK, batchWriteResults.get(0).getStatus());


Nit (you may ignore): I would feel more natural to compare the status before comparing the time.

makes sense -- flipped.

run synthtool to generate batchwrite protos

54ba444

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 28, 2020

thebrianchen self-assigned this May 28, 2020

thebrianchen force-pushed the bc/bulk-commit branch from 68f60a5 to 93a5c73 Compare May 28, 2020 19:24

add bulkCommit()

dcb4f28

thebrianchen force-pushed the bc/bulk-commit branch from 93a5c73 to dcb4f28 Compare May 28, 2020 19:30

thebrianchen requested a review from schmidt-sebastian May 28, 2020 19:37

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen May 28, 2020

BenWhitehead suggested changes May 28, 2020

View reviewed changes

BenWhitehead added the status: blocked Resolving the issue is dependent on other work. label May 28, 2020

BenWhitehead approved these changes May 28, 2020

View reviewed changes

Brian Chen added 2 commits May 28, 2020 17:42

change to use io.grpc.Status

6a1512c

update test to reflect new Status

d820b3a

thebrianchen removed the status: blocked Resolving the issue is dependent on other work. label May 29, 2020

schmidt-sebastian approved these changes May 29, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian May 29, 2020

resolve comments

29f0785

thebrianchen force-pushed the bc/bulk-commit branch from 23dba2d to 29f0785 Compare May 29, 2020 18:42

rerun CI

7363e48

BenWhitehead added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 1, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 1, 2020

thebrianchen merged commit b60ce7f into bc/bulk-master Jun 1, 2020

thebrianchen deleted the bc/bulk-commit branch June 1, 2020 22:56

BenWhitehead pushed a commit that referenced this pull request Aug 7, 2020

fix: add bulkCommit() (#231)

e9af50f

BenWhitehead added a commit that referenced this pull request Aug 7, 2020

merge conflict from #231

40e01bb

BenWhitehead pushed a commit that referenced this pull request Aug 7, 2020

fix: add bulkCommit() (#231)

2b1e063

Conversation

thebrianchen commented May 28, 2020

Uh oh!

codecov bot commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

BenWhitehead left a comment

Choose a reason for hiding this comment

Uh oh!

BenWhitehead commented May 28, 2020

Uh oh!

thebrianchen commented May 28, 2020

Uh oh!

BenWhitehead left a comment

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!

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!

5 participants

codecov bot commented May 28, 2020 •

edited

Loading