Add transaction retries #2250
Add transaction retries #2250
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/retry
branch
from
b6ef023 to
53cde80
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/retry
branch
from
53cde80 to
8afb5e5
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/retry
branch
from
8afb5e5 to
e4cdfe0
Compare
| mode: 'readonly' | 'readwrite', | ||
| idempotent: boolean, |
There was a problem hiding this comment.
The type of this parameter is inconsistent with mode, which is also effectively a boolean. Could you make both of these numeric enums? That would avoid all the /* idempotent= */ comments.
However, see above: I think it's likely better to have idempotent mode variants here too.
There was a problem hiding this comment.
I am using the same mode Strings here now (minus the -primary).
| transactionOperation: ( | ||
| transaction: PersistenceTransaction | ||
| ) => PersistencePromise<T> | ||
| ): Promise<T> { | ||
| log.debug(LOG_TAG, 'Starting transaction:', action); | ||
|
|
||
| const idempotent = |
There was a problem hiding this comment.
How about
const idempotent = mode.endsWith('idempotent');
const readonly = mode.startsWith('readonly');?
There was a problem hiding this comment.
Sure. Remind me of this comment though when someone figures out how to use IndexedDb in IE11, which doesn't support these fancy String APIs :)
| this.networkEnabled, | ||
| this.inForeground | ||
| return this.simpleDb.runTransaction( | ||
| 'readwrite', |
There was a problem hiding this comment.
This is surprising to me. It seems like if SimpleDb implemented the various modes these implementations could remain unchanged.
There was a problem hiding this comment.
I changed this to plumb through this setting via the mode strings. This sounded better in theory then it actually is, as it lead to a lot of string parsing and makes re-constructing the final IDBTransactionMode pretty ugly. Since this change is temporary and reduces the diff of this PR considerably it might be worth the trade off.
| await transaction.completionPromise; | ||
| return transactionFnResult; | ||
| } catch (e) { | ||
| // TODO: We could probably be smarter about this an not retry exceptions |
There was a problem hiding this comment.
TODO(schmidt-sebastian)? or bug number?
| */ | ||
| function isDomException(e: Error): boolean { | ||
| return ( | ||
| (typeof DOMException !== 'undefined' && e instanceof DOMException) || |
There was a problem hiding this comment.
Is this true in our fake IndexedDb implementation too?
There was a problem hiding this comment.
I tried to elude to this in the JavaDoc comment, but moved the comment into the function. typeof DOMException !== 'undefined' is not true in node:persistence, but` e.constructor.name === 'DOMException' is.
| it('retries idempotent transactions', async () => { | ||
| let attemptCount = 0; | ||
|
|
||
| const result = await db.runTransaction( |
There was a problem hiding this comment.
This function isn't actually idempotent and I can't see from reading this why this would actually fail and trigger multiple attempts. Could you add some comments describing your intent here?
There was a problem hiding this comment.
I made the failure more obvious by adding a comment pointing out exactly where it would fail.
As for the idempotency: I think it will be mighty hard to write a unit test for an actually idempotent function that fails on the first attempt but succeeds thereafter. I hope that the intent of the code speaks for itself.
There was a problem hiding this comment.
Sorry I wasn't clear--I don't think the function should be idempotent because I agree that writing a test that succeeds while using one would be hard.
I was trying to say that "retries idempotent transactions" seemed misleading. Also that I couldn't understand how the thing failed, and the test name didn't help me understand that.
You might consider renaming the test to "retries readwrite-idempotent transactions" to emphasize that it's the mode flag that's idempotent and not the function implementing it.
There was a problem hiding this comment.
Got it. I renamed the test to 'retries transactions marked as idempotent' (since 'readwrite-idempotent' is hopefully short lived).
| expect(attemptCount).to.equal(2); | ||
| }); | ||
|
|
||
| it('retries transaction only three times', async () => { |
There was a problem hiding this comment.
Should "transaction" be plural here?
schmidt-sebastian
force-pushed
the
mrschmidt/retry
branch
from
6ed49d3 to
a1e5a00
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/retry
branch
from
a5cf9d3 to
aee20aa
Compare
schmidt-sebastian
changed the base branch from
master
to
mrschmidt/idempotent
|
FYI: Changed this to point at a feature branch, which should allow us to remove most TODOs before merging into master. |
| it('retries idempotent transactions', async () => { | ||
| let attemptCount = 0; | ||
|
|
||
| const result = await db.runTransaction( |
There was a problem hiding this comment.
Sorry I wasn't clear--I don't think the function should be idempotent because I agree that writing a test that succeeds while using one would be hard.
I was trying to say that "retries idempotent transactions" seemed misleading. Also that I couldn't understand how the thing failed, and the test name didn't help me understand that.
You might consider renaming the test to "retries readwrite-idempotent transactions" to emphasize that it's the mode flag that's idempotent and not the function implementing it.
* Add transaction retries (#2250) * Marking SimpleDb calls as idempotent (#2251) * Mark mostly readonly calls as idempotent (#2252) * Fix test failure (#2256) * Make handleUserChange idempotent (#2257) * Temporarily disable CountingQueryEngine tests (#2258) * Improve test hack (#2259) * Improve test hack * Comment in test hack * Make getNewDocumentChanges() idempotent (#2255) * Add onCommitted listeners for transactions (#2265) * Fix build * Fix Lint * Make applyRemoteEvent idempotent (#2263) * Make notifyLocalViewChanges idempotent (#2268) * Make releaseQuery idempotent (#2266) * Mark acknowledgeBatch and rejectBatch idempotent (#2269) * idempotent `allocateQuery` and `notifyLocalViewChanges` (#2264) * Mark collectGarbage idempotent (#2267) * Idempotency: Address TODOs, add Changelog (#2270)
This PR prepares idempotent IndexedDb transactions for Firestore (see go/ios-13).
The first commit is purely a refactor that adds an additional 'idempotent=false' argument to SimpleDb.runTransaction. The second commit adds code that retries transactions up to three times (unconditionally, as long as a transaction function is marked idempotent and we receive some sort of DOMException).