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.

Sign up for GitHub

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: Replace channel provider for fixed channel provider. #3582

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: Replace channel provider for fixed channel provider. #3582

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import com.google.api.gax.rpc.ApiException;
import com.google.api.gax.rpc.ClientContext;
import com.google.api.gax.rpc.FixedHeaderProvider;
import com.google.api.gax.rpc.FixedTransportChannelProvider;
import com.google.api.gax.rpc.HeaderProvider;
import com.google.api.gax.rpc.InstantiatingWatchdogProvider;
import com.google.api.gax.rpc.OperationCallable;
Expand Down Expand Up @@ -402,11 +403,14 @@ public GapicSpannerRpc(final SpannerOptions options) {
final String emulatorHost = System.getenv("SPANNER_EMULATOR_HOST");

try {
FixedTransportChannelProvider fixedChannelProvider =
FixedTransportChannelProvider.create(channelProvider.getTransportChannel());
Comment on lines +406 to +407
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we need a bit more management and logic around this:

  1. We should only use a FixedTransportChannelProvider if the user has not specified a channel provider themselves. That is; We should only do this if (options.getChannelProvider() == null). The reason is that for the default channel provider, we know that it produces a channel pool, which can safely be wrapped in a FixedChannelProvider. We don't know what any custom channel provider that has been set might produce, and whether it is a good idea to just get one channel from that provider and use that for all requests.
  2. We also need to keep a reference to the channel that is returned by defaultChannelProvider.build().getTransportChannel(), and then close that channel when this GapicSpannerRpc instance is shut down. A FixedTransportChannelProvider is not closed when a stub is closed. This will leave the channels open after this instance has been closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree on 2. Will update.

For 1, as a user I would expect that the channel provider that I specified will only be executed once per client, not four times per client. However the current implementation is different and for the sake of not breaking anyone who may rely on that we may keep the existing behaviour for the user-specified channel provider. What do you prefer?

Also, about the test failures. Are those flakes?

Most errors look like

Error:    RetryOnInvalidatedSessionTest....
Spanner FAILED_PRECONDITION: io.grpc.StatusRuntimeException: FAILED_PRECONDITION: This transaction has been invalidated by a later transaction in the same session.
Transaction id: 1
Expected: 2


SpannerStubSettings spannerStubSettings =
options
.getSpannerStubSettings()
.toBuilder()
.setTransportChannelProvider(channelProvider)
.setTransportChannelProvider(fixedChannelProvider)
.setCredentialsProvider(credentialsProvider)
.setStreamWatchdogProvider(watchdogProvider)
.setTracerFactory(
Expand Down Expand Up @@ -446,7 +450,7 @@ public GapicSpannerRpc(final SpannerOptions options) {
.build();
SpannerStubSettings.Builder pdmlSettings = options.getSpannerStubSettings().toBuilder();
pdmlSettings
.setTransportChannelProvider(channelProvider)
.setTransportChannelProvider(fixedChannelProvider)
.setCredentialsProvider(credentialsProvider)
.setStreamWatchdogProvider(watchdogProvider)
.setTracerFactory(
Expand Down Expand Up @@ -476,7 +480,7 @@ public GapicSpannerRpc(final SpannerOptions options) {
options
.getInstanceAdminStubSettings()
.toBuilder()
.setTransportChannelProvider(channelProvider)
.setTransportChannelProvider(fixedChannelProvider)
.setCredentialsProvider(credentialsProvider)
.setStreamWatchdogProvider(watchdogProvider)
.setTracerFactory(
Expand All @@ -489,7 +493,7 @@ public GapicSpannerRpc(final SpannerOptions options) {
options
.getDatabaseAdminStubSettings()
.toBuilder()
.setTransportChannelProvider(channelProvider)
.setTransportChannelProvider(fixedChannelProvider)
.setCredentialsProvider(credentialsProvider)
.setStreamWatchdogProvider(watchdogProvider)
.setTracerFactory(
Expand Down Expand Up @@ -538,7 +542,7 @@ public <RequestT, ResponseT> UnaryCallable<RequestT, ResponseT> createUnaryCalla

// Check whether the SPANNER_EMULATOR_HOST env var has been set, and if so, if the emulator
// is actually running.
checkEmulatorConnection(options, channelProvider, credentialsProvider, emulatorHost);
checkEmulatorConnection(options, fixedChannelProvider, credentialsProvider, emulatorHost);
} catch (Exception e) {
throw newSpannerException(e);
}
Expand Down
Loading