feat: retry BatchGetDocuments RPCs that fail with errors #1544
Conversation
google-cla
bot
added
the
cla: yes
product-auto-label
bot
added
the
api: firestore
schmidt-sebastian
force-pushed
the
mrschmidt/getall
branch
2 times, most recently
from
8baf0a9
to
207d054
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/getall
branch
from
207d054
to
a04a119
Compare
dev/src/document-reader.ts
Outdated
| * as part of a transaction. | ||
| * | ||
| * @param firestore The Firestore instance to use. | ||
| * @param allDocuments The documents to receive. |
dev/src/document-reader.ts
Outdated
| /** An optional transaction ID to use for this read. */ | ||
| transactionId?: Uint8Array; | ||
|
|
||
| private remainingDocuments = new Set<string>(); |
There was a problem hiding this comment.
nit: something more specific like unfetchedDocuments or requestedDocuments?
There was a problem hiding this comment.
Renamed to outstandingDocuments. Pretty outstanding, me thinks.
| * | ||
| * @param requestTag A unique client-assigned identifier for this request. | ||
| */ | ||
| async get(requestTag: string): Promise<Array<DocumentSnapshot<T>>> { |
There was a problem hiding this comment.
If get() is only meant to be run once, the method should throw an error if it is run again
There was a problem hiding this comment.
It is only meant to be run once, but it should work if run multiple times. It will return the same result without doing any work.
dev/src/document-reader.ts
Outdated
| } | ||
|
|
||
| const path = document.ref.formattedName; | ||
| this.remainingDocuments.delete(path); |
There was a problem hiding this comment.
optional: add assert() to check that path is in the set.
There was a problem hiding this comment.
Skipped for now.
dev/src/document-reader.ts
Outdated
| ); | ||
| throw err; | ||
| } else { | ||
| logger( |
There was a problem hiding this comment.
Is the maximum number of retries allowed capped?
There was a problem hiding this comment.
No, I thought it would be better not to. But then I changed my mind. I rewrote this logic to only fetch more results if we made some progress, which should ensure that we never end up in a loop with just errors. This also removes the need to add backoff, which I otherwise should have done.
FYI - This rewrite was pretty complicated, so I ended up changing the implementation to use async iterators. I did this in steps in case you want to see more reasonable diffs.
schmidt-sebastian
force-pushed
the
mrschmidt/getall
branch
from
7ee61e8
to
3634fd6
Compare
dev/src/document-reader.ts
Outdated
| private retrievedDocuments = new Map<string, DocumentSnapshot>(); | ||
|
|
||
| /** | ||
| * Internal method to retrieve multiple documents from Firestore, optionally |
There was a problem hiding this comment.
nit: it seems weird to call the constructor an "internal method". Move this comment elsewhere?
There was a problem hiding this comment.
Oh, that's an old comment. Updated.
schmidt-sebastian
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
🤖 I have created a release \*beep\* \*boop\* --- ## [4.13.0](https://www.github.com/googleapis/nodejs-firestore/compare/v4.12.3...v4.13.0) (2021-06-29) ### Features * add read-only transactions ([#1541](https://www.github.com/googleapis/nodejs-firestore/issues/1541)) ([ca4241e](https://www.github.com/googleapis/nodejs-firestore/commit/ca4241eb3ee4abb8453b6da0911397187dc18dde)) * retry BatchGetDocuments RPCs that fail with errors ([#1544](https://www.github.com/googleapis/nodejs-firestore/issues/1544)) ([b39dd3c](https://www.github.com/googleapis/nodejs-firestore/commit/b39dd3c65549fb1a651c1722d8ea2c038e152417)) ### Bug Fixes * **deps:** google-gax v2.17.0 with mTLS ([#1546](https://www.github.com/googleapis/nodejs-firestore/issues/1546)) ([a322345](https://www.github.com/googleapis/nodejs-firestore/commit/a32234510d487982b950c88575b9425c531c2d94)) * make request optional in all cases ([#1536](https://www.github.com/googleapis/nodejs-firestore/issues/1536)) ([f6edfc1](https://www.github.com/googleapis/nodejs-firestore/commit/f6edfc181ca39cd307eab6d141db08f377d5cfdf)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR moves the existing logic for Firestore.getAll() to a new class and adds a new retry mechanism that is similar to the current Query RPC retry.
Fixes #1387