Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Lazy-started transactions #2017

Merged
merged 22 commits into from
May 5, 2024

Conversation

brettwillis
Copy link
Contributor

@brettwillis brettwillis commented Mar 15, 2024

Eliminates at least one round-trip network request for every transaction by the following

  • Transactions are started lazily upon the first read request (or not at all if no reads) instead of by the initial separate beginTransaction request. All subsequent reads (and commit/rollback) are queued upon the first read operation of each transaction attempt.
  • Empty transactions (no reads or writes) become a network no-op (zero requests, no begin and no commit)
  • Eliminate superfluous rollback request for some types of commit errors (where the transaction is already aborted by the server)
  • Added an internal getResponse API to the DocumentReader, Query and AggregateQuery classes to expose the transaction ID in the response.
  • The internal _stream() APIs now emit a "transaction response" as the first element when appropriate.

Other opportunistic improvements

  • Avoid initialising useless write batch and backoff instances on read-only transactions

Fixes #2015

Copy link

conventional-commit-lint-gcf bot commented Mar 15, 2024

🤖 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 automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label bot added size: l Pull request size is large. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Mar 15, 2024
dev/src/transaction.ts Outdated Show resolved Hide resolved
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Mar 18, 2024
@brettwillis brettwillis changed the title Lazy-started transactions feat: Lazy-started transactions Mar 18, 2024
@brettwillis brettwillis marked this pull request as ready for review March 18, 2024 08:40
@brettwillis brettwillis requested review from a team as code owners March 18, 2024 08:40
@tom-andersen tom-andersen added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 18, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 18, 2024
@tom-andersen tom-andersen self-assigned this Mar 18, 2024
@ehsannas ehsannas added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 18, 2024
Copy link
Contributor

@tom-andersen tom-andersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the quite large contribution. I prefer if the rollback optimization based on error code was moved to a separate PR. I am not convinced that this change is correct. Verifying will hold up this PR. The problem is with reads having created locks, that should be released. If we wait for timeout of transaction, that can create problems for retry and other clients working on same documents.

I am giving you some preliminary feedback, but will return to do a more thorough review.

I sent you a PR to change the tests. For readTime, we do not need a transaction at all. This will eliminate the need to wait for first request to complete with transactionId, and this will also overcome potential transaction timeout.

dev/test/transaction.ts Show resolved Hide resolved
dev/test/transaction.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@tom-andersen tom-andersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I really like your implementation. There are a few things I would like to see changed, but then I see no reason why this shouldn't be approved and merged.

Note, there will be a delay before we can merge since this has to fit into our upcoming release schedule. We should have this out by April.

dev/src/document-reader.ts Outdated Show resolved Hide resolved
dev/src/transaction.ts Show resolved Hide resolved
dev/src/transaction.ts Outdated Show resolved Hide resolved
dev/src/transaction.ts Outdated Show resolved Hide resolved
dev/src/transaction.ts Outdated Show resolved Hide resolved
dev/src/transaction.ts Outdated Show resolved Hide resolved
@brettwillis
Copy link
Contributor Author

There we go. With this last commit GitHub seems to have gotten itself unstuck. I've implemented all of those suggestions. You haven't seen the second-to-last commit Rollback is completed asynchronously, that was the one that was stuck processing.

Make resilient to wether transaction is included in same or different response
Test transaction ID buffer length
@brettwillis
Copy link
Contributor Author

@tom-andersen I ran this branch against a real Firestore backend and oops there were a few gotchas

  • Protobuf includes zero/empty "transaction ID" buffers by default
  • Sometimes the backend returns the "transaction" in the same response stream element as the "document response" and sometimes it doesn't which surprised me based on the docs. So I made it more resilient to which response comes and when.

Warrants another review sorry.

@tom-andersen tom-andersen added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 23, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 23, 2024
@tom-andersen tom-andersen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 25, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 25, 2024
): Promise<QuerySnapshot<AppModelType, DbModelType>> {
return this._queryUtil._get(this, transactionIdOrReadTime) as Promise<
async _get(
transactionIdOrReadTime?: Uint8Array | Timestamp | api.ITransactionOptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/transactionIdOrReadTime/transactionOrReadTime

rollback('foo1'),
query({newTransaction: {readWrite: {}}, error: serverError}),
// No rollback because the lazy-start operation failed
//rollback('foo1'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any value in keeping these //rollback('foo1') ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no transaction id yet, since that is part of the result from first query. So we can't rollback, even if we wanted to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup. I'm just saying we should remove the commented code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh. Good point! +1

Comment on lines 638 to 641
return this._transactionIdPromise.then(async opts => {
const r = await resultFn.call(this, param, opts);
return r.result;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: to make it more consistent with the rest of this function

      return this._transactionIdPromise
                 .then(opts => resultFn.call(this, param, opts))
                 .then(r => r.result);

Copy link
Contributor

@MarkDuckworth MarkDuckworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic. There are many nice code improvements, but it's also a significant community contribution. Thank you for your efforts.

I have a few additional comments for your consideration. But otherwise I think it looks good.

The only potential adverse affect would be if a customer is implementing parallel reads in a transaction using get(), then this has a possibility to slow the transaction if the first read is slow/large-doc. But in many cases that could be mitigated with a call to getAll().

dev/src/reference.ts Show resolved Hide resolved
const docs: Array<QueryDocumentSnapshot<AppModelType, DbModelType>> = [];
const output: Omit<QueryResponse<never>, 'result'> & {
readTime?: Timestamp;
} = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Instead of this complex type definition, it might be more readable code to just construct a new type, or single object definition. Or even just keep a few individual properties.

dev/src/transaction.ts Show resolved Hide resolved
dev/src/transaction.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @brettwillis . LGTM -- only the remaining minor comments should be addressed.

@tom-andersen tom-andersen added the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2024
@tom-andersen tom-andersen added the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2024
@tom-andersen tom-andersen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024
@tom-andersen tom-andersen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 1, 2024
@tom-andersen tom-andersen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 2, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 2, 2024
@MarkDuckworth MarkDuckworth removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 3, 2024
@tom-andersen tom-andersen merged commit 2c726a1 into googleapis:main May 5, 2024
16 checks passed
@brettwillis brettwillis deleted the transaction-lazy-begin branch May 5, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve network latency of runTransaction() routine
5 participants