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 backoff to BulkWriter #600

Merged
merged 3 commits into from Apr 23, 2021
Merged

feat: add backoff to BulkWriter #600

merged 3 commits into from Apr 23, 2021

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.

None yet

2 participants