-
Notifications
You must be signed in to change notification settings - Fork 136
chore(spanner): fix code and tests #3636
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
chore(spanner): fix code and tests #3636
Conversation
| } | ||
|
|
||
| @Test | ||
| public void testRetryUsesPreviousTransactionIdOnMultiplexedSession() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this unit test since it is not possible to change the txnstate to ABORTED of the TransactionManager using unit tests. This is needed since we are calling resetForRetryAsync, which will fail if transactionstate is not ABORTED.
We have same tests covered in mock spanner
Line 75 in c8d1e63
| public class MultiplexedSessionDatabaseClientMockServerTest extends AbstractMockServerTest { |
| } | ||
| } | ||
| dbClient.write(mutations); | ||
| if (mutations.size() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we really need this check? (and if so, could we change is to !mutations.isEmpty())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that at this point, there are no mutations to commit, making the db.write call unnecessary. Adding a check for pending mutations before committing will avoid an unnecessary backend call and save some time in our tests, though the improvement is minimal.
c74369b
into
googleapis:main
No description provided.