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: new transaction feature #1239

Merged

Conversation

danieljbruce
Copy link
Contributor

@danieljbruce danieljbruce commented Mar 4, 2024

Summary

This PR ensures that transaction read calls will use newTransaction options to begin transactions instead of making a separate beginTransaction grpc call.

Source code changes

A #blockWithMutex function is added and it is used for read calls instead of a #withBeginTransaction function. This makes it so that all read calls on Transaction will block on the mutex and then call the same method in their super class. Before this change, the read calls on Transaction would block on the mutex, make a beginTransaction grpc call and then call the same method in their super class. So this change to using #blockWithMutex omits the extra beginTransaction call and instead expects the function it is calling to supply new transaction options.

A function getTransactionRequest is added which pulls out the code used for building transaction options for a beginTransaction request. That is because this code is also used to build the new transaction options.

TransactionState is moved up to the super class DatastoreRequest and a new state is added called NOT_TRANSACTION for DataRequests that are not transactions.

parseTransactionResponse is moved up to the super class DatastoreRequest and is now used by runQuery, runAggregationQuery and get to save the transaction id.

newTransaction options are now built in getRequestOptions which is used by runQuery, runAggregationQuery and get to save the transaction id.

state is moved up to the DatastoreRequest super class because it now has to be used to decide whether or not to build newTransaction options.

If a transaction is expired then an error will be thrown when the transaction is used.

ReadOptions are the options shared by Datastore read requests.
new_transaction is a consistency type in ReadOptions. When new_transaction is used in a read request, a new transaction is begun for the request.

System tests added

A system test is added to ensure using new transaction has better latency. System tests are added for runQuery, runAggregationQuery and get for readOnly transactions to ensure readOnly transactions work correctly whether transaction.run is called or not.

Unit tests added

Unit tests are added for various orders of transaction operations that ensure correct behaviour for when transaction.run is used and when transaction.run is omitted. lookup, lookup, put, commit is one order that is tested. The others are runQuery, lookup, put, commit, runAggregationQuery, lookup, put, commit, put, put, lookup, commit and put, commit. A unit test is also corrected.

Drawbacks

If the user uses transaction.runQueryStream or they use transaction.createReadStream then the call will not block on a mutex. These methods are not async methods so they cannot wait for a #blockWithMutex call to complete. If users are currently using runQueryStream or createReadStream concurrently then these multiple read calls may be beginning multiple transactions at the same time using one transaction object which is not ideal.

Transaction state should move up to the super class
Add a method for building transaction options. Also add a method for detecting if a request is a transaction. Build new transaction options
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: datastore Issues related to the googleapis/nodejs-datastore API. labels Mar 4, 2024
Corrected getTransactionRequest. Build the request options properly.
When this system test isn’t set to be a readOnly transaction then the error occurs which says too much contention.
Change parseRunSuccess so that it can be used more universally.
parseRunSuccess should move up to the super class because it now needs to be used there.
This change saves the transaction id returned from the server for read calls
Make sure that code using the new transaction option has better performance than code that doesn’t have it.
Use the MockedTransactionWrapper to test requests being passed into the Gapic layer.
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Mar 6, 2024
Mock out begin transaction. Test for newTransaction consistency type. Mock out commit.
A transaction.run test is needed  for lookup, lookup, put, commit.
Add a test for this sequence of operations and ensure it works properly.
Four operations that get all the results for running an aggregation query. Adding another test for these four operations.
Last test suite regarding new transaction unit tests.
Add a bunch of tests for the commit case. Check the commit gapic input.
@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 6, 2024
Begin transaction should be called at least once. Add code here to increment the counter.
Remove commented code too.
@danieljbruce danieljbruce changed the title New transaction feature branch feat: new transaction feature branch Mar 7, 2024
Check for expired on most functions and write tests for the expired check.
@@ -68,35 +53,13 @@ describe('Run Query', () => {
}
}

it('should pass read time into runQuery for transactions', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this test is no longer valid because transaction and read time are no longer compatible together as is the case in Python.

throw Error(transactionExpiredError);
}
}

/**
* Retrieve the entities as a readable object stream.
*

Choose a reason for hiding this comment

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

Nit: update L381 with new added errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done.

callback(new Error(transactionExpiredError));
return;
}
if (this.state === TransactionState.NOT_STARTED) {

Choose a reason for hiding this comment

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

Just wondering what are the allowed states here? NOT_TRANSACTION and IN_PROGRESS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOT_TRANSACTION, NOT_STARTED, IN_PROGRESS, EXPIRED.

src/request.ts Outdated
if (this.state === TransactionState.EXPIRED) {
callback(new Error(transactionExpiredError));
return;
}

Choose a reason for hiding this comment

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

Do we need to check readTimeAndConsistencyError here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. That check is done further downstream in the call to runQueryStream. But I did spot some other cases where errors weren't bubbling up and fixed those bugs.

Fix the concurrency tests and move the error reporting outside of the stream, add two tests for the streams to make sure errors get reported.
This makes it so that it is reported in the streams.
@danieljbruce danieljbruce changed the title feat: new transaction feature branch feat: new transaction feature Jun 18, 2024
Copy link

@daniel-sanche daniel-sanche left a comment

Choose a reason for hiding this comment

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

LGTM

export function getTransactionRequest(
transaction: Transaction,
options: RunOptions
): TransactionRequestOptions {

Choose a reason for hiding this comment

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

please add some comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/request.ts Outdated
if (id) {
reqOpts.readWrite = {previousTransaction: id};
}
}

Choose a reason for hiding this comment

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

is there a way to trim down this logic a bit? Seems a bit redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I generated unit tests, corrected them and then reduced this function down to one ternary operation. Previously this was just copy pasted code from before. Search describe('getTransactionRequest', () => { to see how this function should behave.

danieljbruce and others added 7 commits June 20, 2024 11:36
I am going to rewrite the getTransactionRequest function so it is a good idea not to change how it works.
Fix the compiler error
Change code so that when readOnly is specified then readwrite options are ignored.
Fix the unit test to adopt this change.
Add comments, simplify logic in the function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the googleapis/nodejs-datastore API. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants