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
Conversation
ac26884
to
b094577
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOperation.java
Show resolved
Hide resolved
@Override | ||
@Nonnull | ||
public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) { | ||
int expected = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
google-cloud-firestore/src/test/java/com/google/cloud/firestore/BulkWriterTest.java
Show resolved
Hide resolved
@Override | ||
@Nonnull | ||
public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) { | ||
if (delay > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed.
google-cloud-firestore/src/main/java/com/google/cloud/firestore/BulkWriterOperation.java
Show resolved
Hide resolved
google-cloud-firestore/src/test/java/com/google/cloud/firestore/BulkWriterTest.java
Show resolved
Hide resolved
🤖 I have created a release \*beep\* \*boop\* --- ## [2.3.0](https://www.github.com/googleapis/java-firestore/compare/v2.2.7...v2.3.0) (2021-04-23) ### Features * add backoff to BulkWriter ([#600](https://www.github.com/googleapis/java-firestore/issues/600)) ([e295aa5](https://www.github.com/googleapis/java-firestore/commit/e295aa5d20007a513e1647575f6935e243825c4d)) ### Dependencies * update dependency com.google.cloud:google-cloud-shared-dependencies to v1 ([#607](https://www.github.com/googleapis/java-firestore/issues/607)) ([21e8cde](https://www.github.com/googleapis/java-firestore/commit/21e8cde718b5f2e2f8269d860d0ea3ae810dabdd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Combination of 1 and 2 from node.