-
Notifications
You must be signed in to change notification settings - Fork 113
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): add R/W multiplexed session support #3381
Conversation
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClientImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DelayedAsyncRunner.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DelayedAsyncRunner.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public TransactionContextFuture beginAsync() { | ||
return getAsyncTransactionManager().beginAsync(); |
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.
Same here as above: Preferably, these methods should be non-blocking if possible.
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.
Modifying this to be non-blocking would require the method signature to be modified, which makes it impossible here. Should we just accept this as it is which means this will be blocking until a multiplexed session is available.
Please let me know if there are any other ways to handle this?
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'm not sure I understand why the signature would need to change. The return value is a TransactionContextFuture
(so something that extends Future
). That being said; If it requires a lot of work or refactoring, then it is certainly not worth it, so I'm fine with leaving this as is.
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.
We might need to use ApiFutures.transformAsync
to make beginAsync()
to be non-blocking. However, the return type of transacformAsync is a ApiFuture<O>
which does not conform to the return type of this method which is TransactionContextFuture
. So I am assuming the signature need to be changed to ApiFuture<TransactionContextFuture>
.
However, since TransactionContextFuture extends Future
, just typecasting could be sufficient.
That said, let me pick up this analysis in a seperate PR to get the rest of PRs unblocking.
...oud-spanner/src/main/java/com/google/cloud/spanner/DelayedMultiplexedSessionTransaction.java
Outdated
Show resolved
Hide resolved
...oud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestWithClosedSessionsEnv.java
Outdated
Show resolved
Hide resolved
...r/src/test/java/com/google/cloud/spanner/MultiplexedSessionDatabaseClientMockServerTest.java
Outdated
Show resolved
Hide resolved
...r/src/test/java/com/google/cloud/spanner/MultiplexedSessionDatabaseClientMockServerTest.java
Outdated
Show resolved
Hide resolved
...r/src/test/java/com/google/cloud/spanner/MultiplexedSessionDatabaseClientMockServerTest.java
Outdated
Show resolved
Hide resolved
...r/src/test/java/com/google/cloud/spanner/MultiplexedSessionDatabaseClientMockServerTest.java
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClientImpl.java
Show resolved
Hide resolved
|
||
@Override | ||
public TransactionContextFuture beginAsync() { | ||
return getAsyncTransactionManager().beginAsync(); |
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'm not sure I understand why the signature would need to change. The return value is a TransactionContextFuture
(so something that extends Future
). That being said; If it requires a lot of work or refactoring, then it is certainly not worth it, so I'm fine with leaving this as is.
This PR introduces support for using multiplexed sessions in R/W transactions for the following methods: