fix: read time should be used for transaction reads #1171
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Latency is caused by the call to getProjectId from Google auth. This change allows the project id to be retrieved if it is set in the client at creation time thereby reducing call latency.
A test file is created where we mock out commit in the Gapic layer. The mock allows us to get the results passed to the commit endpoint in the Gapic layer.
To prove that the change works to reduce latency, a test is written. The test checks to see that the amount of time that passes between the time when the initial call is made in the user’s code and the time when the call reaches the gapic layer is sufficiently small. It will be a very small amount of time if the program does not need to do an auth lookup.
Run the linter so that spacing in the PR gets fixed for some of the lines of code.
The license header needs to be added to the top of the new test file that is used for mocking out commit.
This is going to be a test for investigating the latency of the client.
Measure the latency between the original call and the mock server.
Do check external to function after async call. Add log for call time.
Other mock doesn’t require lazy client initialization.
Eliminate the fake datastore client because we need to do assertion checks that are specific to each test. This means there is no point in defining runQuery once in a mock because each test will mock it out differently.
Add the code change that will add read time to read options for transactions. # Conflicts: # test/transaction.ts
The idea is to test that read time got passed along for transactions specifically. This will be necessary for snapshot reads to work.
Need the entire test suite to run
The before hook is not necessary. Just mock out the data client at the start.
Files were cherry-picked that weren’t helpful for solving the problem. Remove them.
Right now, providing a transaction id is necessary to run the request as a transaction.
The integration test looks at the data from the snapshot read time for transactions and ensures that the read has no data thereby exercising the read time parameter.
…into not-use-read-time-for-run-tx # Conflicts: # system-test/datastore.ts
Fix the indents in the system test folder
…ieljbruce/nodejs-datastore into not-use-read-time-for-run-tx
beginTransaction needs to be mocked out now that a transaction will begin if runQuery is called.
Add a sleep. Instead of changing the current test, add a new test because it means the reader of the PR can be sure that test coverage wasn’t reduced which is better.
Modify the test so that sleeps are long enough to create predictable results and tests actually check for the right values.
The test setup sometimes prepares before data with 0 entries and sometimes prepares before data with 1 entry so a less restrictive test is required in order for it to consistently pass.
bhshkh
approved these changes
Apr 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now, read calls with a transaction do not use the provided snapshot read time. The code change in src ensures that read options always gets passed along to the Gapic layer. Read options contain the read time so ensuring read options are passed along to the gapic layer ensures that read time is passed along to the gapic layer and sent in the grpc call to be used.
A separate unit test file was created for the test because all of the current unit test files have mocks that we don't want to use. Mocking out functions in the Gapic layer is becoming a common use case so we create a file that allows us to mock out runQuery for each test and write a test there with the intention of writing more tests there in the future.