fix: bulkWriter: writing to the same document does not create a new batch#1298
fix: bulkWriter: writing to the same document does not create a new batch#1298thebrianchen merged 8 commits intomasterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1298 +/- ##
=======================================
Coverage 98.45% 98.46%
=======================================
Files 30 30
Lines 18675 18648 -27
Branches 1442 1435 -7
=======================================
- Hits 18387 18362 -25
+ Misses 285 283 -2
Partials 3 3
Continue to review full report at Codecov.
|
schmidt-sebastian
left a comment
There was a problem hiding this comment.
This is mostly good, though I wonder if we need to fix the "docsToRetry" logic in the WriteBatch constructor?
dev/src/bulk-writer.ts
Outdated
| this.pendingOps.get(result.key)!.reject(result.status); | ||
| this.pendingOps.delete(result.key); | ||
| for (let i = resolvedOps.length - 1; i >= 0; i--) { | ||
| this.pendingOps.splice(resolvedOps[i], 1); |
There was a problem hiding this comment.
Aren't these indices off after the first splice? I wonder if we can simply build up the reverse array in the loop above and replace this.pendingOps with it.
There was a problem hiding this comment.
Since we're going in reverse order and deleting the last element in the array each time, the remaining indices won't be affected.
There was a problem hiding this comment.
Can you add a comment?
dev/test/bulk-writer.ts
Outdated
| }); | ||
|
|
||
| it('sends writes to the same document in separate batches', async () => { | ||
| it('sends writes to the same documents in the same batch', async () => { |
There was a problem hiding this comment.
Does this test still have value over the other tests?
There was a problem hiding this comment.
This test would fail if pendingOps got changed back to its original map type since the second write to the same document path would override the existing entry, so it serves to make sure that writes to the same document are added properly.
There was a problem hiding this comment.
Makes sense. Maybe prefix the test name with "can send"?
schmidt-sebastian
left a comment
There was a problem hiding this comment.
Thanks for the updates. Can you find a way to address the comment in the summary response?
Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
a404d4a to
a578792
Compare
Updated to use an index-based tracking system instead. Also modified the retry test to include retries to the same document path, which would have failed with the previous iteration. |
dev/src/bulk-writer.ts
Outdated
| this.pendingOps.delete(result.key); | ||
| private processResults( | ||
| results: BatchWriteResult[], | ||
| failRemainingOperations = false |
There was a problem hiding this comment.
I think this should be "allowRetry" or something like that, which also means that you can just merge the corresponding branch with if (!this.shouldRetry(result.status.code)). In the context of this method, failRemainingOperations is not quite self-explanatory.
dev/src/write-batch.ts
Outdated
| // indexes to retry. | ||
| this._ops = retryBatch._ops.filter( | ||
| v => docsToRetry!.indexOf(v.docPath) !== -1 | ||
| (op, index) => indexesToRetry!.indexOf(index) !== -1 |
There was a problem hiding this comment.
This is a bit confusing to read. Could we make indexesToRetry a Set? Optional.
Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
No description provided.