fix: retry ABORTED writes in bulkCommit #1243
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1243 +/- ##
==========================================
- Coverage 98.50% 98.50% -0.01%
==========================================
Files 30 30
Lines 18545 18624 +79
Branches 1422 1431 +9
==========================================
+ Hits 18268 18345 +77
- Misses 274 276 +2
Partials 3 3
Continue to review full report at Codecov.
|
dev/src/bulk-writer.ts
Outdated
| import {Deferred, wrapError} from './util'; | ||
| import {Deferred, getRetryCodes, wrapError} from './util'; | ||
| import {BatchWriteResult, WriteBatch, WriteResult} from './write-batch'; | ||
| import {Status} from 'google-gax'; |
There was a problem hiding this comment.
Nit: External imports should go in the first block.
dev/src/bulk-writer.ts
Outdated
| error?: Error | ||
| ): number[] { | ||
| const indexesToRetry: number[] = []; | ||
| if (error) { |
There was a problem hiding this comment.
Can you add a small comment here to explain what this is doing?
| try { | ||
| const results = await this.writeBatch.bulkCommit(); | ||
| retryIndexes = this.processResults(originalIndexMap, results); | ||
| } catch (err) { |
There was a problem hiding this comment.
I tried to see if I could get rid of this re-assignment and got completely carried away. Here is a cleanup:
https://gist.github.com/schmidt-sebastian/702ba33ce0c3515d096089226a762934
Use at your own risk and take at your own consideration :)
There was a problem hiding this comment.
Huge fan of this organization. I implemented what you did, but changed keys to docPaths since I was a bit confused about what the keys were at first. I think this approach will work with the UpdateBuilder pattern on java as well.
dev/src/bulk-writer.ts
Outdated
| this.completedDeferred.resolve(); | ||
|
|
||
| if (indexesToRetry.length === 0) { | ||
| this.completedDeferred.resolve(); |
There was a problem hiding this comment.
We also need to resolve this when all retry attempts failed. Please also add a test for this.
schmidt-sebastian
left a comment
There was a problem hiding this comment.
Thanks for the update. Some naming/comment nits to ruin your weekend.
dev/src/bulk-writer.ts
Outdated
| results = await this.writeBatch.bulkCommit(); | ||
| } catch (err) { | ||
| // Map the failure to each individual write's result. | ||
| results = [...this.pendingOps.keys()].map(key => { |
There was a problem hiding this comment.
Should "key" be "path" then?
dev/src/bulk-writer.ts
Outdated
| } else { | ||
| for (let i = 0; i < this.opCount; i++) { | ||
| this.resultsMap.get(i)!.reject(error); | ||
| processResults(results: BatchWriteResult[], shouldRetry: boolean): void { |
There was a problem hiding this comment.
s/shouldRetry/mayRetry (or allowRetry)?
Also might be worth adding @param comments.
There was a problem hiding this comment.
N/A since we're using failRemainingOperations.
dev/src/bulk-writer.ts
Outdated
| } | ||
| } | ||
|
|
||
| this.processResults(results, /* shouldRetry= */ false); |
There was a problem hiding this comment.
This might be cleaner if this was this.failRemainingOperations(results) and a separate call.
There was a problem hiding this comment.
I originally didn't want to copy/paste code, but in this case, it clarifies the behavior much better.
| this.completedDeferred.resolve(); | ||
| return; |
There was a problem hiding this comment.
This could just be a break, this.processResult() would be a no-op.
There was a problem hiding this comment.
The map entry is deleted from pendingOps after the promise is resolved in processResults, so an error would be thrown if processResults is called again.
dev/src/bulk-writer.ts
Outdated
| return code !== undefined && retryCodes.includes(code); | ||
| } | ||
|
|
||
| has(path: string): boolean { |
There was a problem hiding this comment.
Let's use a more descriptive name here (hasDoc(), hasWrite(), ...).
| constructor(firestore: Firestore); | ||
| constructor( | ||
| firestore: Firestore, | ||
| retryBatch?: WriteBatch, |
There was a problem hiding this comment.
Please document.
| constructor( | ||
| firestore: Firestore, | ||
| retryBatch?: WriteBatch, | ||
| docsToRetry?: string[] |
There was a problem hiding this comment.
Please document.
Recreating #1222 to merge against PR without deleting comment history.