Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions google-cloud-spanner/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>grpc-gcp</artifactId>
<version>1.7.0</version>
</dependency>
<dependency>
<groupId>io.grpc</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,6 @@ public class SpannerOptions extends ServiceOptions<Spanner, SpannerOptions> {
private final InstanceAdminStubSettings instanceAdminStubSettings;
private final DatabaseAdminStubSettings databaseAdminStubSettings;
private final Duration partitionedDmlTimeout;
private final boolean grpcGcpExtensionEnabled;
private final GcpManagedChannelOptions grpcGcpOptions;
private final boolean autoThrottleAdministrativeRequests;
private final RetrySettings retryAdministrativeRequestsSettings;
Expand Down Expand Up @@ -798,7 +797,6 @@ protected SpannerOptions(Builder builder) {
throw SpannerExceptionFactory.newSpannerException(e);
}
partitionedDmlTimeout = builder.partitionedDmlTimeout;
grpcGcpExtensionEnabled = builder.grpcGcpExtensionEnabled;
grpcGcpOptions = builder.grpcGcpOptions;
autoThrottleAdministrativeRequests = builder.autoThrottleAdministrativeRequests;
retryAdministrativeRequestsSettings = builder.retryAdministrativeRequestsSettings;
Expand Down Expand Up @@ -1025,7 +1023,6 @@ public static class Builder
private DatabaseAdminStubSettings.Builder databaseAdminStubSettingsBuilder =
DatabaseAdminStubSettings.newBuilder();
private Duration partitionedDmlTimeout = Duration.ofHours(2L);
private boolean grpcGcpExtensionEnabled = false;
private GcpManagedChannelOptions grpcGcpOptions;
private RetrySettings retryAdministrativeRequestsSettings =
DEFAULT_ADMIN_REQUESTS_LIMIT_EXCEEDED_RETRY_SETTINGS;
Expand Down Expand Up @@ -1097,7 +1094,6 @@ protected Builder() {
this.instanceAdminStubSettingsBuilder = options.instanceAdminStubSettings.toBuilder();
this.databaseAdminStubSettingsBuilder = options.databaseAdminStubSettings.toBuilder();
this.partitionedDmlTimeout = options.partitionedDmlTimeout;
this.grpcGcpExtensionEnabled = options.grpcGcpExtensionEnabled;
this.grpcGcpOptions = options.grpcGcpOptions;
this.autoThrottleAdministrativeRequests = options.autoThrottleAdministrativeRequests;
this.retryAdministrativeRequestsSettings = options.retryAdministrativeRequestsSettings;
Expand Down Expand Up @@ -1573,14 +1569,12 @@ public Builder enableGrpcGcpExtension() {
* Multiplexed sessions are not supported for gRPC-GCP.
*/
public Builder enableGrpcGcpExtension(GcpManagedChannelOptions options) {
this.grpcGcpExtensionEnabled = true;

Choose a reason for hiding this comment

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

medium

This line is correctly removed. However, with gRPC-GCP now enabled by default, the method name enableGrpcGcpExtension is misleading as it only configures options. Consider deprecating this method and introducing a new one with a more descriptive name like setGrpcGcpOptions to improve API clarity.

this.grpcGcpOptions = options;
return this;
}

/** Disables gRPC-GCP extension. */
public Builder disableGrpcGcpExtension() {
this.grpcGcpExtensionEnabled = false;

Choose a reason for hiding this comment

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

medium

While removing this line is correct as part of making gRPC-GCP always enabled, the disableGrpcGcpExtension() method now has no effect. This can be misleading for users. The method should be deprecated to clearly communicate that it's a no-op and the extension cannot be disabled.

It would be best to modify the method on the right side to be:

    /**
     * Disables gRPC-GCP extension.
     *
     * @deprecated gRPC-GCP is now enabled by default and can no longer be disabled. This method is a no-op.
     */
    @java.lang.Deprecated
    public Builder disableGrpcGcpExtension() {
      return this;
    }

return this;
}

Expand Down Expand Up @@ -1793,8 +1787,7 @@ public SpannerOptions build() {
credentials = environment.getDefaultExperimentalHostCredentials();
}
if (this.numChannels == null) {
this.numChannels =
this.grpcGcpExtensionEnabled ? GRPC_GCP_ENABLED_DEFAULT_CHANNELS : DEFAULT_CHANNELS;
this.numChannels = GRPC_GCP_ENABLED_DEFAULT_CHANNELS;
}

synchronized (lock) {
Expand Down Expand Up @@ -1989,7 +1982,7 @@ public Duration getPartitionedDmlTimeoutDuration() {
}

public boolean isGrpcGcpExtensionEnabled() {
return grpcGcpExtensionEnabled;
return true;
}

public GcpManagedChannelOptions getGrpcGcpOptions() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,6 @@ public class GapicSpannerRpc implements SpannerRpc {
private final boolean leaderAwareRoutingEnabled;
private final boolean endToEndTracingEnabled;
private final int numChannels;
private final boolean isGrpcGcpExtensionEnabled;

private final GrpcCallContext baseGrpcCallContext;

Expand Down Expand Up @@ -335,7 +334,6 @@ public GapicSpannerRpc(final SpannerOptions options) {
this.leaderAwareRoutingEnabled = options.isLeaderAwareRoutingEnabled();
this.endToEndTracingEnabled = options.isEndToEndTracingEnabled();
this.numChannels = options.getNumChannels();
this.isGrpcGcpExtensionEnabled = options.isGrpcGcpExtensionEnabled();
this.baseGrpcCallContext = createBaseCallContext();

if (initializeStubs) {
Expand Down Expand Up @@ -592,10 +590,6 @@ private static GcpManagedChannelOptions grpcGcpOptionsWithMetrics(SpannerOptions
private static void maybeEnableGrpcGcpExtension(
InstantiatingGrpcChannelProvider.Builder defaultChannelProviderBuilder,
final SpannerOptions options) {
if (!options.isGrpcGcpExtensionEnabled()) {
return;
}

final String jsonApiConfig = parseGrpcGcpApiConfig();
final GcpManagedChannelOptions grpcGcpOptions = grpcGcpOptionsWithMetrics(options);

Expand Down Expand Up @@ -2037,20 +2031,14 @@ <ReqT, RespT> GrpcCallContext newCallContext(
GrpcCallContext context = this.baseGrpcCallContext;
Long affinity = options == null ? null : Option.CHANNEL_HINT.getLong(options);
if (affinity != null) {
if (this.isGrpcGcpExtensionEnabled) {
// Set channel affinity in gRPC-GCP.
// Compute bounded channel hint to prevent gRPC-GCP affinity map from getting unbounded.
int boundedChannelHint = affinity.intValue() % this.numChannels;
context =
context.withCallOptions(
context
.getCallOptions()
.withOption(
GcpManagedChannel.AFFINITY_KEY, String.valueOf(boundedChannelHint)));
} else {
// Set channel affinity in GAX.
context = context.withChannelAffinity(affinity.intValue());
}
// Set channel affinity in gRPC-GCP.
// Compute bounded channel hint to prevent gRPC-GCP affinity map from getting unbounded.
int boundedChannelHint = affinity.intValue() % this.numChannels;
context =
context.withCallOptions(
context
.getCallOptions()
.withOption(GcpManagedChannel.AFFINITY_KEY, String.valueOf(boundedChannelHint)));
}
if (options != null) {
// TODO(@odeke-em): Infer the affinity if it doesn't match up with in the request-id.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,9 @@ public class ChannelUsageTest {
@Parameter(0)
public int numChannels;

@Parameter(1)
public boolean enableGcpPool;

@Parameters(name = "num channels = {0}, enable GCP pool = {1}")
@Parameters(name = "num channels = {0}")
public static Collection<Object[]> data() {
return Arrays.asList(
new Object[][] {{1, true}, {1, false}, {2, true}, {2, false}, {4, true}, {4, false}});
return Arrays.asList(new Object[][] {{1}, {2}, {4}});
}

private static final Statement SELECT1 = Statement.of("SELECT 1 AS COL1");
Expand Down Expand Up @@ -208,27 +204,10 @@ private SpannerOptions createSpannerOptions() {
.build())
.setHost("http://" + endpoint)
.setCredentials(NoCredentials.getInstance());
if (enableGcpPool) {
builder.enableGrpcGcpExtension();
}

return builder.build();
}

@Test
public void testCreatesNumChannels() {
try (Spanner spanner = createSpannerOptions().getService()) {
assumeFalse(
"GRPC-GCP is currently not supported with multiplexed sessions",
isMultiplexedSessionsEnabled(spanner) && enableGcpPool);
DatabaseClient client = spanner.getDatabaseClient(DatabaseId.of("p", "i", "d"));
try (ResultSet resultSet = client.singleUse().executeQuery(SELECT1)) {
while (resultSet.next()) {}
}
}
assertEquals(numChannels, batchCreateSessionLocalIps.size());
}

@Test
public void testUsesAllChannels() throws InterruptedException {
final int multiplier = 2;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1100,9 +1100,10 @@ public void testDefaultNumChannelsWithGrpcGcpExtensionDisabled() {
SpannerOptions.newBuilder()
.setProjectId("test-project")
.setCredentials(NoCredentials.getInstance())
.disableGrpcGcpExtension()

Choose a reason for hiding this comment

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

medium

The name of this test, testDefaultNumChannelsWithGrpcGcpExtensionDisabled, is now misleading because the gRPC-GCP extension can no longer be disabled. Since this test now verifies the behavior when the no-op disableGrpcGcpExtension() method is called, consider renaming it to something like testDefaultNumChannelsWhenDisableGrpcGcpExtensionIsCalled for clarity.

.build();

assertEquals(SpannerOptions.DEFAULT_CHANNELS, options.getNumChannels());
assertEquals(SpannerOptions.GRPC_GCP_ENABLED_DEFAULT_CHANNELS, options.getNumChannels());
}

@Test
Expand Down Expand Up @@ -1135,9 +1136,10 @@ public void testNumChannelsWithGrpcGcpExtensionEnabled() {

@Test
public void checkCreatedInstanceWhenGrpcGcpExtensionDisabled() {
SpannerOptions options = SpannerOptions.newBuilder().setProjectId("test-project").build();
SpannerOptions options =
SpannerOptions.newBuilder().setProjectId("test-project").disableGrpcGcpExtension().build();
Comment on lines +1139 to +1140

Choose a reason for hiding this comment

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

medium

The name of this test, checkCreatedInstanceWhenGrpcGcpExtensionDisabled, is now misleading as the gRPC-GCP extension is always enabled. To improve clarity, consider renaming the test to reflect that it's verifying behavior when the no-op disableGrpcGcpExtension() is called, for example: checkCreatedInstanceWhenDisableGrpcGcpExtensionIsCalled.

SpannerOptions options1 = options.toBuilder().build();
assertEquals(false, options.isGrpcGcpExtensionEnabled());
assertEquals(true, options.isGrpcGcpExtensionEnabled());
assertEquals(options.isGrpcGcpExtensionEnabled(), options1.isGrpcGcpExtensionEnabled());

Spanner spanner1 = options.getService();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ public void testTransactionRunner_usesSingleChannel() {
return null;
});
}
System.out.println("streamingReadLocalIps: " + streamingReadLocalIps);
assertEquals(1, streamingReadLocalIps.size());
}
}
Loading