Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add buffering layer to BulkWriter #611

Merged
merged 2 commits into from Apr 29, 2021
Merged

Conversation

thebrianchen
Copy link

Porting from node

@thebrianchen thebrianchen self-assigned this Apr 27, 2021
@thebrianchen thebrianchen requested a review from a team as a code owner April 27, 2021 17:04
@thebrianchen thebrianchen requested a review from a team April 27, 2021 17:04
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Apr 27, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 27, 2021
@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #611 (114fc26) into master (325452e) will increase coverage by 0.07%.
The diff coverage is 89.74%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #611      +/-   ##
============================================
+ Coverage     73.96%   74.04%   +0.07%     
+ Complexity     1106     1102       -4     
============================================
  Files            67       67              
  Lines          5866     5898      +32     
  Branches        718      721       +3     
============================================
+ Hits           4339     4367      +28     
- Misses         1302     1305       +3     
- Partials        225      226       +1     
Impacted Files Coverage Δ Complexity Δ
...in/java/com/google/cloud/firestore/BulkWriter.java 98.47% <89.74%> (-1.53%) 55.00 <6.00> (+5.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 325452e...114fc26. Read the comment docs.

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!

}

return operation.getFuture();
ApiFuture<WriteResult> transformedFuture =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A slightly less generic name would do wonders here :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to processedOperationFuture.

processBufferedOperations();
return ApiFutures.immediateFuture(result);
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this not need an executor for the non-deprecated version?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added executor, thanks for catching!

@thebrianchen thebrianchen added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Apr 28, 2021
@yoshi-kokoro yoshi-kokoro removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Apr 29, 2021
@thebrianchen thebrianchen merged commit a7caff2 into master Apr 29, 2021
@thebrianchen thebrianchen deleted the bc/bulk-buffer branch April 29, 2021 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/java-firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants