fix: Add BulkWriter #233

thebrianchen · 2020-06-03T00:20:05Z

Porting from node (multiple commits).

TODO:

Add POJO support.

codecov · 2020-06-03T00:21:49Z

Codecov Report

Merging #233 into bc/bulk-master will increase coverage by 0.10%.
The diff coverage is 81.13%.

@@                 Coverage Diff                  @@
##             bc/bulk-master     #233      +/-   ##
====================================================
+ Coverage             73.62%   73.72%   +0.10%     
- Complexity             1048     1087      +39     
====================================================
  Files                    65       67       +2     
  Lines                  5650     5866     +216     
  Branches                622      642      +20     
====================================================
+ Hits                   4160     4325     +165     
- Misses                 1296     1341      +45     
- Partials                194      200       +6

Impacted Files	Coverage Δ	Complexity Δ
.../com/google/cloud/firestore/BulkWriterOptions.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...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.00 <0.00> (ø)`
...m/google/cloud/firestore/v1/FirestoreSettings.java	`16.36% <0.00%> (ø)`	`4.00 <0.00> (ø)`
...le/cloud/firestore/v1/stub/FirestoreAdminStub.java	`5.88% <0.00%> (ø)`	`1.00 <0.00> (ø)`
.../google/cloud/firestore/v1/stub/FirestoreStub.java	`5.88% <0.00%> (ø)`	`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> (ø)`
...java/com/google/cloud/firestore/FirestoreImpl.java	`78.19% <50.00%> (-0.44%)`	`25.00 <1.00> (-1.00)`
...oogle/cloud/firestore/v1/FirestoreAdminClient.java	`57.46% <50.00%> (ø)`	`38.00 <23.00> (ø)`
... and 36 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 b60ce7f...05ab240. Read the comment docs.

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

thebrianchen · 2020-06-03T00:33:35Z

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

+public class BulkWriterOptions {
+  private final boolean disableThrottling;
+
+  BulkWriterOptions(boolean disableThrottling) {


I noticed that all the other options objects have private constructors and expose create() instead. Should I do this here as well?

The idea behind these classes is to offer "named arguments". In this case, we would want to have a static factory method that is called "disableThrottling" or "withThrottlingDisabled". I would suggest to look at SetOptions rather than at TransactionOptions for inspiration :)

Thanks, added withThrottlingDisabled(). Given that the BulkWriter will default to throttling when no options object is passed in, do you see value in adding a withThrottlingEnabled() option? I can't think of a use case for it, but want to make sure.

schmidt-sebastian

I didn't have time to look at tests yet, will do before lunch.

schmidt-sebastian · 2020-06-03T03:20:39Z

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

+   */
+  @Nonnull
+  public ApiFuture<WriteResult> delete(
+      @Nonnull DocumentReference documentReference, @Nonnull Precondition options) {


s/options/precondition

Also: Precondition should be singular. Only a single option can be set.

If it is not too much work, maybe we can fix this in UpdateBuilder too.

I originally had precondition but changed it to options to match UpdateBuilder.

Changed to precondition here and in UpdateBuilder.

schmidt-sebastian · 2020-06-03T03:22:26Z

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

+   * Delete a document from the database.
+   *
+   * @param documentReference The DocumentReference to delete.
+   * @return An ApiFuture containing the result of the delete. Throws an error if the delete fails.


We don't throw an error, we return an error via the ApiFuture.

changed to contains.

schmidt-sebastian · 2020-06-03T03:29:50Z

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

+                flushComplete.set(null);
+              }
+            },
+            MoreExecutors.directExecutor());


I am actually not sure what directExecutor refers to here. Do you mind doing a quick verification for me?
If a consumer calls flush().get() does it resolve? If MoreExecutors.directExecutor() schedules the runnable on the user thread, then it will block. It likely gets executed on GRPC's thread (the thread that resolves the bulk commits), in which case we are ok.

If the executor scheduled the runnable on the user thread, wouldn't the unit test calls to flush.get() hang indefinitely?

From what I've researched, directExecutor() will run the task in the thread that invokes execute (link). I'm not sure how to determine which thread is responsible for that, but from what I've seen in the codebase, most if not all calls to ApiFutures.transform() are called with directExecutor.

I verified that this is running on the GAX executor in the transaction code, so this pattern is fine (and your research validated). Thanks for digging this up!

FWIW I don't think we can judge threading behavior with our unit tests. Our integration tests are great for that.

Thanks for checking :)

schmidt-sebastian · 2020-06-03T03:31:59Z

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

+      // Send the batch if it is under the rate limit, or schedule another attempt after the
+      // appropriate timeout.
+      long delayMs = rateLimiter.getNextRequestDelayMs(batch.getOpCount());
+      Preconditions.checkArgument(delayMs != -1, "Batch size should be under capacity");


s/checkArgument/checkState

schmidt-sebastian · 2020-06-03T03:34:53Z

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

+    boolean success = rateLimiter.tryMakeRequest(batch.getOpCount());
+    Preconditions.checkState(success, "Batch should be under rate limit to be sent.");
+    try {
+      List<BatchWriteResult> results = batch.bulkCommit().get();


You are blocking on the bulkCommit() here. You need to add a listener to proceed with the batch result logic instead of calling get(). Look at ApiFutures for convenience methods that help with listener regristration.

Moved logic to a listener, but I can't find a way to implement this without 2 separate try/catch blocks. I need the outer one in case bulkCommit() throws an error, and the inner one is required to call .get().

schmidt-sebastian · 2020-06-03T16:52:28Z

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

 * WriteBatch}.
 */
-public abstract class UpdateBuilder<T extends UpdateBuilder> {
+public abstract class UpdateBuilder<T> {


This implementation is now very different from Node. Do you think we can easily get Node to match? I am a little hesitant to provide two drastically different reference implementations.

I think we could probably get node to match, but my fear is that it would result in more work for the contractors on top of all the other changes that BulkWriter requires. I took a look at the python and php admin SDKs and they follow the current node pattern. Java seems unique in using the UpdateBuilder pattern. Thoughts?

Let's leave as is. Java remains outlier. We need to point that out in our docs for the contractors.

will make a note.

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

schmidt-sebastian · 2020-06-03T16:53:44Z

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

+    return state;
+  }
+
+  HashSet<String> getDocPaths() {


Please return Set as returning a HashSet ties us to this implementation.

schmidt-sebastian · 2020-06-03T16:53:55Z

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

+    return docPaths;
+  }
+
+  int getOpCount() {


Java doesn't use abbreviations.

renamed to getOperationCount

schmidt-sebastian · 2020-06-03T16:55:01Z

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

+  /** Resolves the individual operations in the batch with the results. */
+  void processResults(List<BatchWriteResult> results, @Nullable Exception error) {
+    if (error == null) {
+      for (int i = 0; i < getOpCount(); ++i) {


You are looping over the resultsMap, but this is not apparent. This would be easier to read if you inlined getOpCount here and below (and maybe we can remove it altogether?).

Moved the if-else statements into a single for-loop and switched to using resultsMap.size().

BulkWriter uses getOperationCount() to calculate the RateLimiter delay, so we can't remove the method unless we expose the entire resultsMap variable.

schmidt-sebastian · 2020-06-03T17:59:32Z

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

+  private DocumentReference doc1;
+  private DocumentReference doc2;
+
+  private ApiFuture<BatchWriteResponse> successResponse(int seconds) {


Maybe s/seconds/updateTimeSeconds/

schmidt-sebastian · 2020-06-03T18:00:30Z

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

+  private ApiFuture<BatchWriteResponse> failResponse() {
+    BatchWriteResponse.Builder response = BatchWriteResponse.newBuilder();
+    response.addWriteResultsBuilder().build();
+    response.addStatusBuilder().setCode(14).build();


Use the Code enum here?

schmidt-sebastian · 2020-06-03T18:03:18Z

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

+          {
+            put(
+                batchWrite(set(LocalFirestoreHelper.SINGLE_FIELD_PROTO, "coll/doc1")),
+                failResponse());


Should we pass the error code to failResponse?

Nit: I think the name should be failedResponse, since it returns a failed response.

done. I had considered providing the option for an error code, but I don't see a reason to pass in different error codes in the tests.

schmidt-sebastian · 2020-06-03T18:06:57Z

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

+          {
+            put(
+                batchWrite(create(LocalFirestoreHelper.SINGLE_FIELD_PROTO, "coll/doc1")),
+                successResponse(2));


Use 1 here?

schmidt-sebastian · 2020-06-03T18:07:40Z

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

+          {
+            put(
+                batchWrite(set(LocalFirestoreHelper.SINGLE_FIELD_PROTO, "coll/doc1")),
+                successResponse(0));


IMHO 1 is a better starting point since it is different from the default timestamp.

done here and throughout.

schmidt-sebastian · 2020-06-03T18:17:52Z

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

+                successResponse(3));
+          }
+        };
+    requestResponseMap.initializeStub(batchWriteCapture, firestoreMock, true);


What is true?

separated out into separate class, N/A now.

schmidt-sebastian · 2020-06-03T18:20:00Z

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

+    // timeout.
+
+    List<Integer> arrayRange = new ArrayList<>();
+    for (int i = 0; i < 500; ++i) {


Can we "compute" this value inline?

I tried researching this, but it seems that the clean implementations such as IntStream require Java 8 (https://stackoverflow.com/questions/16020741/shortest-way-of-filling-an-array-with-1-2-n). The other option involves creating a Set with a Range, which doesn't seem so straightforward.

I don't see why we need it. There is only one place where we are looping over arrayRange and we could just use for (int i = 0; i < 500; ++i) instead.

I somehow had it in my head that I needed to have a range of numbers. Oops...

schmidt-sebastian · 2020-06-03T18:23:30Z

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

    return fromJsonString(json.replace("'", "\""));
  }
+
+  static class RequestResponseMap


I feel this should no longer be called RequestResponseMap, this does much more now. Can you come up with a more specific name?

I also think we could split this into two classes - the base RequestResponseMap and a subclass that enforces the one active request setting. With good naming (tm) it would now be super obvious what is going on.

split into two classes with an attempt at naming.

schmidt-sebastian · 2020-06-03T18:27:51Z

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

+
+    void awaitLatch() {
+      try {
+        latch.await();


You should try to generalized this code if you keep it in this generalized helper. A good way to do this would be a BlockingQueue of requests. This could be awaitRequest and use BlockingQueue.poll() to wait for the next request. You would supply the requests in the stub handler (and call BlockingQueue.offer()).

Does this modified implementation work? I'm still using a single latch and future, but it can process multiple serial requests.

If I follow this code correctly, you cannot wait for n requests or buffer n requests. You have to send, wait, send, wait, send and so on. If I send 2 requests, I should be able to invoke awaitRequest() twice without blocking.

I changed it from CountDownLatch to Semaphore, and I think you can buffer multiple requests now. Buffering n requests releases n permits that n awaitRequest() calls can instantly consume without blocking.

schmidt-sebastian · 2020-06-03T18:28:44Z

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

+    SettableApiFuture<Void> activeRequestComplete = SettableApiFuture.create();
+    CountDownLatch latch = new CountDownLatch(1);
+
+    void markComplete() {


Please use more descriptive name.

changed to markInitialRequestComplete.

thebrianchen

thanks for the review!

thebrianchen · 2020-06-03T19:02:25Z

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

+   * Delete a document from the database.
+   *
+   * @param documentReference The DocumentReference to delete.
+   * @return An ApiFuture containing the result of the delete. Throws an error if the delete fails.


changed to contains.

thebrianchen · 2020-06-03T19:03:08Z

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

+   */
+  @Nonnull
+  public ApiFuture<WriteResult> delete(
+      @Nonnull DocumentReference documentReference, @Nonnull Precondition options) {


I originally had precondition but changed it to options to match UpdateBuilder.

Changed to precondition here and in UpdateBuilder.

thebrianchen · 2020-06-03T19:06:25Z

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

+      // Send the batch if it is under the rate limit, or schedule another attempt after the
+      // appropriate timeout.
+      long delayMs = rateLimiter.getNextRequestDelayMs(batch.getOpCount());
+      Preconditions.checkArgument(delayMs != -1, "Batch size should be under capacity");


thebrianchen · 2020-06-03T19:08:13Z

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

+    // Remove the batch from BatchQueue after it has been processed.
+    int batchIndex = batchQueue.indexOf(batch);
+    Preconditions.checkState(batchIndex != -1, "The batch should be in the BatchQueue.");
+    batchQueue.remove(batchIndex);


thebrianchen · 2020-06-03T19:09:18Z

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

+   * READY_TO_SEND (2) not write to references that are currently in flight.
+   */
+  private boolean isBatchSendable(BulkCommitBatch batch) {
+    if (batch.getState() != UpdateBuilder.BatchState.READY_TO_SEND) {


thebrianchen · 2020-06-03T22:07:16Z

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

+        bulkWriter.create(
+            firestoreMock.document("coll/doc3"), LocalFirestoreHelper.SINGLE_FIELD_MAP);
+
+    // The 4th write should not be sent because it should be in a new batch. However, this write


comment typo thanks.

thebrianchen · 2020-06-03T22:08:27Z

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

+                successResponse(3));
+          }
+        };
+    requestResponseMap.initializeStub(batchWriteCapture, firestoreMock, true);


separated out into separate class, N/A now.

thebrianchen · 2020-06-03T22:08:46Z

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

+
+    // Wait for flush() to perform logic and reach the stubbed response. This simulates a first
+    // batch that has been sent with its response still pending.
+    requestResponseMap.awaitLatch();


changed to awaitRequest.

thebrianchen · 2020-06-03T22:14:44Z

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

+    // timeout.
+
+    List<Integer> arrayRange = new ArrayList<>();
+    for (int i = 0; i < 500; ++i) {


I tried researching this, but it seems that the clean implementations such as IntStream require Java 8 (https://stackoverflow.com/questions/16020741/shortest-way-of-filling-an-array-with-1-2-n). The other option involves creating a Set with a Range, which doesn't seem so straightforward.

thebrianchen · 2020-06-03T23:51:58Z

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

+
+    void awaitLatch() {
+      try {
+        latch.await();


Does this modified implementation work? I'm still using a single latch and future, but it can process multiple serial requests.

schmidt-sebastian

Some drive-by comments. More thorough review later tonight.

schmidt-sebastian · 2020-06-04T23:57:47Z

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

  private final boolean disableThrottling;

-  BulkWriterOptions(boolean disableThrottling) {
+  private BulkWriterOptions(boolean disableThrottling) {


Denver would be very happy if the internal API used enableThrottling. I would prefer that too!

schmidt-sebastian · 2020-06-04T23:58:05Z

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


  /**
-   * Create a set of options to disable or enable throttling.
+   * An option object that will disable throttling in the created BulkWriter.


s/option/options/

schmidt-sebastian · 2020-06-04T23:59:39Z

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

  }

+  /**
+   * Wraps the result of the write operation before it is returned.


Maybe add: 'WrapResult is used to generate the return value for all public methods.'

added in 2nd paragraph.

schmidt-sebastian · 2020-06-05T00:00:54Z

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


-  static class RequestResponseMap
+  /** Map of request/response pairs to be returned when `sendRequest()` is called. */
+  static class RequestResponseStubber


Nit: This only stubs responses. Maybe ResponseStubber?

It also seems like the comment is no longer applicable.

renamed and updated comment.

schmidt-sebastian · 2020-06-05T00:01:21Z

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

+   *
+   * <p>Enforces that only one active request can be pending at a time.
+   */
+  static class SerialRequestStubber extends RequestResponseStubber {


Similarly, this could be SerialResponseStubber.

schmidt-sebastian · 2020-06-05T00:02:18Z

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

    CountDownLatch latch = new CountDownLatch(1);

-    void markComplete() {
+    void markRequestComplete() {


Maybe markAllRequestsComplete()? Not sure.

Done. That name makes sense if we are allowing multiple requests.

schmidt-sebastian · 2020-06-05T00:02:30Z

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

    }

-    void awaitLatch() {
+    void awaitRequest() {


awaitNextRequest?

Shouldn't it be await (this) request (the request that you just made)? You would call awaitRequest() after you've made a request first, and not the other way around.

schmidt-sebastian · 2020-06-05T00:03:35Z

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

      } catch (Exception e) {
        fail("latch.await() should not fail");
      }
+      latch = new CountDownLatch(1);


I think you have to use semaphores here to allow for multiple requests to build up and be consumed. The state needs to span the lifetime of the Stubber.

schmidt-sebastian

Thanks for the updates! Especially the test helper is now much cleaner. Some final nit as always :)

schmidt-sebastian · 2020-06-05T03:59:16Z

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

+                flushComplete.set(null);
+              }
+            },
+            MoreExecutors.directExecutor());


I verified that this is running on the GAX executor in the transaction code, so this pattern is fine (and your research validated). Thanks for digging this up!

FWIW I don't think we can judge threading behavior with our unit tests. Our integration tests are great for that.

schmidt-sebastian · 2020-06-05T04:01:08Z

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

+    this.disableThrottling = disableThrottling;
+  }
+
+  boolean getDisableThrottling() {


This should be isThrottlingEnabled

schmidt-sebastian · 2020-06-05T04:06:35Z

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

+    // timeout.
+
+    List<Integer> arrayRange = new ArrayList<>();
+    for (int i = 0; i < 500; ++i) {


I don't see why we need it. There is only one place where we are looping over arrayRange and we could just use for (int i = 0; i < 500; ++i) instead.

schmidt-sebastian · 2020-06-05T04:10:03Z

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

+
+    void awaitLatch() {
+      try {
+        latch.await();


If I follow this code correctly, you cannot wait for n requests or buffer n requests. You have to send, wait, send, wait, send and so on. If I send 2 requests, I should be able to invoke awaitRequest() twice without blocking.

schmidt-sebastian · 2020-06-05T04:12:25Z

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

+              @Override
+              public ApiFuture<? extends GeneratedMessageV3> answer(
+                  InvocationOnMock invocationOnMock) throws Throwable {
+                return checkOneActiveRequestAndReturnResponse(response);


If you make checkOneActiveRequestAndReturnResponse() a validateRequest() method that in the base class doesn't do anything and in this class is checkOneActiveRequestAndReturnResponse(), then you can have a single shared implementation of initializeStub().

Great suggestion, thanks!

schmidt-sebastian · 2020-06-07T22:36:37Z

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

+   */
+  static class ResponseStubber
      extends LinkedHashMap<GeneratedMessageV3, ApiFuture<? extends GeneratedMessageV3>> {
+    ApiFuture<? extends GeneratedMessageV3> checkOneActiveRequestAndReturnResponse(


This method should:

have a generic name

be documented

(ignore the previous version of this comment)

Renamed to verifyResponse and added comments.

schmidt-sebastian · 2020-06-07T22:38:57Z

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

+            };
+        stubber = (stubber != null) ? stubber.doAnswer(answer) : doAnswer(answer);
+      }
+      if (stubber != null) {


If stubber is null here, how would the tests work? Seems like we should assert that stubber is not null.

Good point, the if statement is a lazy solution. Added a checkNotNull

schmidt-sebastian

LGTM with final nits.

fix: Add BulkWriter

b623f11

thebrianchen self-assigned this Jun 3, 2020

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

thebrianchen commented Jun 3, 2020

View reviewed changes

google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriter.java Show resolved Hide resolved

thebrianchen commented Jun 3, 2020

View reviewed changes

cleanup

8611796

thebrianchen requested a review from schmidt-sebastian June 3, 2020 01:10

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jun 3, 2020

schmidt-sebastian reviewed Jun 3, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jun 3, 2020

resolve comments rd. 1

98984a4

thebrianchen commented Jun 3, 2020

View reviewed changes

thebrianchen requested a review from schmidt-sebastian June 4, 2020 00:00

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jun 4, 2020

schmidt-sebastian reviewed Jun 5, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jun 5, 2020

resolve comments rd. 2

1040402

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jun 7, 2020

thebrianchen requested a review from schmidt-sebastian June 7, 2020 05:37

schmidt-sebastian reviewed Jun 7, 2020

View reviewed changes

schmidt-sebastian approved these changes Jun 7, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jun 7, 2020

resolve comments rd. 3

05ab240

thebrianchen merged commit 64007a2 into bc/bulk-master Jun 8, 2020

thebrianchen deleted the bc/bulk-refactor branch June 8, 2020 13:22

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

fix: Add BulkWriter (#233)

655d362

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

fix: Add BulkWriter (#233)

db07700

fix: Add BulkWriter #233

fix: Add BulkWriter #233

Uh oh!

Conversation

thebrianchen commented Jun 3, 2020

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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!

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!

schmidt-sebastian Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

codecov bot commented Jun 3, 2020 •

edited

Loading

schmidt-sebastian Jun 4, 2020 •

edited

Loading