fix: retry BulkWriter deletes that fail with RST_STREAM error #1442
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thebrianchen
force-pushed
the
bc/bulk-delete-retry
branch
from
a17fcb4
to
1a3f6d6
Compare
google-cla
bot
added
the
cla: yes
product-auto-label
bot
added
the
api: firestore
Codecov Report
@@ Coverage Diff @@
## master #1442 +/- ##
=======================================
Coverage 98.20% 98.20%
=======================================
Files 32 32
Lines 19507 19509 +2
Branches 1363 1363
=======================================
+ Hits 19157 19159 +2
Misses 346 346
Partials 4 4
Continue to review full report at Codecov.
|
dev/src/bulk-writer.ts
Outdated
| @@ -316,10 +316,14 @@ export class BulkWriter { | |||
| * @private | |||
| */ | |||
| private _errorFn: (error: BulkWriterError) => boolean = error => { | |||
| const isRstStreamError = | |||
| (error.code as number) === Status.INTERNAL && | |||
There was a problem hiding this comment.
- We should be able to retry Status.INTERNAL regardless of whether this is a RST_STREAM error.
- We should only add this retry for deletes.
There was a problem hiding this comment.
Would you please explain why the retry should be only for deletes?
The RST could happen on any other methods, this limitation comes from idempotency?
For example, Spanner automatically retries transaction commits, regardless of the queries within the txn. I wonder if there is a firestore specific thing here.
added 2 commits
March 8, 2021 16:15
schmidt-sebastian
approved these changes
Mar 8, 2021
dev/src/bulk-writer.ts
Outdated
| @@ -316,10 +316,13 @@ export class BulkWriter { | |||
| * @private | |||
| */ | |||
| private _errorFn: (error: BulkWriterError) => boolean = error => { | |||
| const isRetryableDeleteError = | |||
| error.operationType === 'delete' && | |||
| (error.code as number) === Status.INTERNAL; | |||
| const retryCodes = getRetryCodes('batchWrite'); | |||
| return ( | |||
| error.code !== undefined && | |||
There was a problem hiding this comment.
Do we actually need this undefined check? Would retryCodes.include() just return false?
gcf-merge-on-green bot
pushed a commit
that referenced
this pull request
Mar 9, 2021
🤖 I have created a release \*beep\* \*boop\* --- ### [4.9.7](https://www.github.com/googleapis/nodejs-firestore/compare/v4.9.6...v4.9.7) (2021-03-09) ### Bug Fixes * export v1 and v1beta1 client class types correctly ([#1445](https://www.github.com/googleapis/nodejs-firestore/issues/1445)) ([6c9319e](https://www.github.com/googleapis/nodejs-firestore/commit/6c9319ed6e2ac0dfe0fcf45853f0b38dc0784686)) * retry BulkWriter deletes that fail with RST_STREAM error ([#1442](https://www.github.com/googleapis/nodejs-firestore/issues/1442)) ([cccf48d](https://www.github.com/googleapis/nodejs-firestore/commit/cccf48de4963403a2e77ba241641a2b77fb993da)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This was referenced Mar 12, 2021
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.