-
Notifications
You must be signed in to change notification settings - Fork 132
chore: fallback for Partitioned Operations with multiplexed session #3710
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: fallback for Partitioned Operations with multiplexed session #3710
Conversation
…tiplexed session. If PartitionQueryRequest or PartitionReadRequest with multiplexed session return unimplemented error, then an implicit fallback to regular session will occur.
…ultiplexed-session-fallback
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.
LGTM, with some minor nits
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BatchClientImpl.java
Outdated
Show resolved
Hide resolved
throw e; | ||
} | ||
} | ||
|
||
void maybeMarkUnimplementedForPartitionedOps(SpannerException spannerException) { | ||
boolean maybeMarkUnimplementedForPartitionedOps(SpannerException spannerException) { |
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 think that we should mark this entire method as synchronized. In theory, the partitionRead
and partitionQuery
methods could be executed in parallel, and if an UNIMPLEMENTED error is returned, we would only want to create the fallback transaction once.
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.
Right. Have synchronized on session.
session.setFallbackSessionReference(sessionClient.createSession().getSessionReference()); | ||
sessionName = session.getName(); | ||
initFallbackTransaction(); |
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.
This should only be executed if it had not already been executed (see also the comment above on making the method synchronized)
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.
Right. Added a validation of unimplementedForPartitionedOps before executing fallback.
If PartitionQueryRequest or PartitionReadRequest with multiplexed session return unimplemented error, then an implicit fallback to regular session will occur.