fix: Optimize Transaction PITR#2002
Conversation
product-auto-label
Bot
added
size: l
tom-andersen
changed the title
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
| requestTag: this._requestTag, | ||
| }) | ||
| .then(() => {}); | ||
| async commit(): Promise<void> { |
There was a problem hiding this comment.
This is purely a refactor to use async syntax.
| .then(resp => { | ||
| this._transactionId = resp.transaction!; | ||
| }); | ||
| const resp = await this._firestore.request< |
There was a problem hiding this comment.
This is purely a refactor to use async syntax.
There was a problem hiding this comment.
Thanks. Nice improvement.
| expect(snapshot.exists).to.be.true; | ||
| expect(snapshot.get('foo')).to.equal(1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I added client side exceptions to attempting write on readonly transaction, so this integration test is no longer valid.
MarkDuckworth
left a comment
MarkDuckworth
left a comment
There was a problem hiding this comment.
Direction looks good. There are a few comments for your consideration.
| ): Transaction { | ||
| if (this._readOnly) { | ||
| throw new Error(READ_ONLY_WRITE_ERROR_MSG); | ||
| } |
There was a problem hiding this comment.
There has been a recent movement in the SDKs to leave runtime validation up to the server. In this case, I think performing the validation in the client is acceptable because a read-only transaction should never be writable, that will not change. Calling this out for your consideration.
There was a problem hiding this comment.
For ReadTime transactions, we don't call commit which would normally be when backend could respond. So this is less preference, and more technical requirement.
| .then(resp => { | ||
| this._transactionId = resp.transaction!; | ||
| }); | ||
| const resp = await this._firestore.request< |
There was a problem hiding this comment.
Thanks. Nice improvement.
| }) | ||
| .then(() => {}); | ||
| async commit(): Promise<void> { | ||
| if (this._readTime) { |
There was a problem hiding this comment.
Is it possible to check if the _readOnly flag is set instead of _readTime. The type safety will not guarantee runTransaction will be called for a readOnly transaction without readTime being set.
There was a problem hiding this comment.
ReadOnly but without ReadTime are still transactions that have BeginTransaction and Commit request. That is why I have to use _readTime, since only in the subset of cases where the transaction if ReadOnly AND with ReadTime can we do the optimization.
However, this is also a little weird of a check that I added, since this will only occur if there is a bug in SDK. The customer never has an opportunity to call commit() on transactions. Calling commit() is done implicitly when update lambda completes. So this should never happen so long as our SDK logic is correct.
| async runTransactionOnce<T>( | ||
| updateFunction: (transaction: Transaction) => Promise<T> | ||
| ): Promise<T> { | ||
| if (!this._readTime) { |
There was a problem hiding this comment.
Same comment with respect to using _readOnly vs _readTime
There was a problem hiding this comment.
Again this optimization only works for the subset of ReadOnly transaction that have ReadTime.
| ); | ||
| } | ||
| const result = await promise; | ||
| if (!this._readTime) { |
2 participants
For transactions with read time, the begin transaction and commit can be omitted, thereby decreasing latency.
b/324245706