KEMBAR78
feat: add backoff to BulkWriter by thebrianchen · Pull Request #600 · googleapis/java-firestore · GitHub
Skip to content

Conversation

@thebrianchen
Copy link

Combination of 1 and 2 from node.

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

codecov bot commented Apr 21, 2021

Codecov Report

Merging #600 (3e8736e) into master (029f01d) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #600      +/-   ##
============================================
+ Coverage     73.85%   73.96%   +0.11%     
- Complexity     1088     1097       +9     
============================================
  Files            67       67              
  Lines          5840     5866      +26     
  Branches        717      720       +3     
============================================
+ Hits           4313     4339      +26     
  Misses         1302     1302              
  Partials        225      225              
Impacted Files Coverage Δ Complexity Δ
...va/com/google/cloud/firestore/BulkCommitBatch.java 100.00% <100.00%> (ø) 7.00 <1.00> (ø)
...in/java/com/google/cloud/firestore/BulkWriter.java 100.00% <100.00%> (ø) 50.00 <4.00> (+5.00)
...om/google/cloud/firestore/BulkWriterOperation.java 100.00% <100.00%> (ø) 9.00 <4.00> (+4.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 029f01d...3e8736e. 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.

Some questions about the tests, but implementation LGTM.

@Override
@Nonnull
public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) {
int expected =
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unused.

Copy link
Author

Choose a reason for hiding this comment

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

removed.

@Override
@Nonnull
public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) {
if (delay > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above.

Copy link
Author

Choose a reason for hiding this comment

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

This test increments retryAttempts so it's ok.

});
bulkWriter.create(doc1, LocalFirestoreHelper.SINGLE_FIELD_MAP);
bulkWriter.flush();
bulkWriter.set(doc1, LocalFirestoreHelper.SINGLE_FIELD_MAP);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a different doc?

Copy link
Author

Choose a reason for hiding this comment

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

changed.

@schmidt-sebastian schmidt-sebastian removed their assignment Apr 23, 2021
@thebrianchen thebrianchen merged commit e295aa5 into master Apr 23, 2021
@thebrianchen thebrianchen deleted the bc/bulk-backoff branch April 23, 2021 22:05
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.

2 participants