fix: lower batch size on BulkWriter retry #688
Conversation
google-cla
bot
added
the
cla: yes
product-auto-label
bot
added
the
api: firestore
| @@ -82,6 +83,9 @@ | |||
| /** The maximum number of writes that can be in a single batch. */ | |||
| public static final int MAX_BATCH_SIZE = 20; | |||
|
|
|||
| /** The maximum number of writes can be can in a single batch that is being retried. */ | |||
There was a problem hiding this comment.
This is Sebastian-level grammar. Please upgrade to Brian-level grammar.
| // A backoff duration greater than 0 implies that this batch is a retry. | ||
| // Retried writes are sent with a batch size of 10 in order to guarantee | ||
| // that the batch is under the 10MiB limit. | ||
| boolean retryInSmallerBatch = |
There was a problem hiding this comment.
This boolean does more than just check whether a smaller batch should be used. It also checks the size of the current batch. I think we need to move some logic or change the name.
| @@ -1062,7 +1075,11 @@ private void sendOperationLocked( | |||
| bulkCommitBatch.enqueueOperation(op); | |||
| enqueueOperationOnBatchCallback.apply(bulkCommitBatch); | |||
|
|
|||
| if (bulkCommitBatch.getMutationsSize() == maxBatchSize) { | |||
| if (op.getBackoffDuration() > 0) { | |||
There was a problem hiding this comment.
I am a bit confused. Why do we need this and retryInSmallerBatch? Is it possible to remove one of these checks?
There was a problem hiding this comment.
The first check involving retryInSmallerBatch ensures that we are adding the retry to an appropriate batch. For example, if the batch already had 15 writes enqueued, and sendOperationLocked() is called with a retry, BulkWriter needs to first send the existing batch to create a new batch of size 10.
Consolidated the checks into a single if statement at the start of the function.
| bulkCommitBatch.setMaxBatchSize(RETRY_MAX_BATCH_SIZE); | ||
| } | ||
|
|
||
| if (bulkCommitBatch.getMutationsSize() == bulkCommitBatch.getMaxBatchSize()) { |
There was a problem hiding this comment.
... especially since we are reading the value right back here.
thebrianchen
force-pushed
the
bc/bulk-smaller-batch
branch
from
1d98f52
to
3cd69f3
Compare
🤖 I have created a release \*beep\* \*boop\* --- ### [2.6.2](https://www.github.com/googleapis/java-firestore/compare/v2.6.1...v2.6.2) (2021-07-29) ### Bug Fixes * Add shopt -s nullglob to dependencies script ([8f4b199](https://www.github.com/googleapis/java-firestore/commit/8f4b199c1dfdf268723e11696733fe5fb6bd5c64)) * Ensures bundles are encoded as UTF8 bytes. ([#695](https://www.github.com/googleapis/java-firestore/issues/695)) ([0946a17](https://www.github.com/googleapis/java-firestore/commit/0946a170a963f50ec77409291c02696f2c416edb)) * lower batch size on BulkWriter retry ([#688](https://www.github.com/googleapis/java-firestore/issues/688)) ([146b21d](https://www.github.com/googleapis/java-firestore/commit/146b21dd6d5772bfd9e023dbf5a1147b29076cdd)) * Update dependencies.sh to not break on mac ([#694](https://www.github.com/googleapis/java-firestore/issues/694)) ([8f4b199](https://www.github.com/googleapis/java-firestore/commit/8f4b199c1dfdf268723e11696733fe5fb6bd5c64)) ### Documentation * began merging variant client samples ([#696](https://www.github.com/googleapis/java-firestore/issues/696)) ([0a10dd8](https://www.github.com/googleapis/java-firestore/commit/0a10dd85de02647a9d08f41d45ebc25ee2689a52)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
No description provided.