feat: make grpc-gcp default enabled #4239
+222
−138
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -840,6 +840,14 @@ CloseableIterator<PartialResultSet> startStream( | |
| request.setTransaction(selector); | ||
| } | ||
| this.ensureNonNullXGoogRequestId(); | ||
| this.incrementXGoogRequestIdAttempt(); | ||
| Map<SpannerRpc.Option, ?> txChannelHint = getTransactionChannelHint(); | ||
| if (txChannelHint != null && txChannelHint.get(Option.CHANNEL_HINT) != null) { | ||
| long channelHint = Option.CHANNEL_HINT.getLong(txChannelHint); | ||
| this.xGoogRequestId.setChannelId(channelHint); | ||
| } else { | ||
| this.xGoogRequestId.setChannelId(session.getChannel()); | ||
| } | ||
|
Comment on lines
+843
to
+850
Collaborator
There was a problem hiding this comment. Hmm.... This is going to conflict with #4263. That PR removes most of the manual handling of Request ID. |
||
| SpannerRpc.StreamingCall call = | ||
| rpc.executeQuery( | ||
| request.build(), | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| [ | ||
| { | ||
| "name": "com.google.cloud.grpc.proto.ApiConfig", | ||
| "allDeclaredFields": true, | ||
| "allDeclaredMethods": true, | ||
| "allDeclaredConstructors": true | ||
| }, | ||
| { | ||
| "name": "com.google.cloud.grpc.proto.ApiConfig$Builder", | ||
| "allDeclaredFields": true, | ||
| "allDeclaredMethods": true, | ||
| "allDeclaredConstructors": true | ||
| }, | ||
| { | ||
| "name": "com.google.cloud.grpc.proto.ChannelPoolConfig", | ||
| "allDeclaredFields": true, | ||
| "allDeclaredMethods": true, | ||
| "allDeclaredConstructors": true | ||
| }, | ||
| { | ||
| "name": "com.google.cloud.grpc.proto.ChannelPoolConfig$Builder", | ||
| "allDeclaredFields": true, | ||
| "allDeclaredMethods": true, | ||
| "allDeclaredConstructors": true | ||
| }, | ||
| { | ||
| "name": "com.google.cloud.grpc.proto.MethodConfig", | ||
| "allDeclaredFields": true, | ||
| "allDeclaredMethods": true, | ||
| "allDeclaredConstructors": true | ||
| }, | ||
| { | ||
| "name": "com.google.cloud.grpc.proto.MethodConfig$Builder", | ||
| "allDeclaredFields": true, | ||
| "allDeclaredMethods": true, | ||
| "allDeclaredConstructors": true | ||
| }, | ||
| { | ||
| "name": "com.google.cloud.grpc.proto.AffinityConfig", | ||
| "allDeclaredFields": true, | ||
| "allDeclaredMethods": true, | ||
| "allDeclaredConstructors": true | ||
| }, | ||
| { | ||
| "name": "com.google.cloud.grpc.proto.AffinityConfig$Builder", | ||
| "allDeclaredFields": true, | ||
| "allDeclaredMethods": true, | ||
| "allDeclaredConstructors": true | ||
| }, | ||
| { | ||
| "name": "com.google.cloud.grpc.proto.AffinityConfig$Command", | ||
| "allDeclaredFields": true, | ||
| "allDeclaredMethods": true, | ||
| "allDeclaredConstructors": true | ||
| } | ||
| ] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,4 +2,5 @@ Args = --initialize-at-build-time=com.google.cloud.spanner.IntegrationTestEnv,\ | |
| org.junit.experimental.categories.CategoryValidator,\ | ||
| org.junit.validator.AnnotationValidator,\ | ||
| java.lang.annotation.Annotation \ | ||
| -H:ReflectionConfigurationResources=${.}/com.google.cloud.spanner/grpc-gcp-reflect-config.json \ | ||
| --features=com.google.cloud.spanner.nativeimage.SpannerFeature | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,10 +17,9 @@ | |
| package com.google.cloud.spanner; | ||
|
|
||
| import static com.google.cloud.spanner.DisableDefaultMtlsProvider.disableDefaultMtlsProvider; | ||
| import static java.util.stream.Collectors.toSet; | ||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| import com.google.cloud.NoCredentials; | ||
| import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult; | ||
|
|
@@ -32,7 +31,6 @@ | |
| import com.google.spanner.v1.StructType; | ||
| import com.google.spanner.v1.StructType.Field; | ||
| import com.google.spanner.v1.TypeCode; | ||
| import io.grpc.Context; | ||
| import io.grpc.Contexts; | ||
| import io.grpc.Metadata; | ||
|
|
@@ -70,13 +68,9 @@ public class ChannelUsageTest { | |
| @Parameter(0) | ||
| public int numChannels; | ||
|
|
||
| @Parameters(name = "num channels = {0}") | ||
| public static Collection<Object[]> data() { | ||
| return Arrays.asList(new Object[][] {{1}, {2}, {4}}); | ||
| } | ||
|
|
||
| private static final Statement SELECT1 = Statement.of("SELECT 1 AS COL1"); | ||
|
|
@@ -106,9 +100,9 @@ public static Collection<Object[]> data() { | |
| private static MockSpannerServiceImpl mockSpanner; | ||
| private static Server server; | ||
| private static InetSocketAddress address; | ||
| // Track channel hints (from X-Goog-Spanner-Request-Id header) per RPC method | ||
| private static final Set<Long> batchCreateSessionChannelHints = ConcurrentHashMap.newKeySet(); | ||
| private static final Set<Long> executeSqlChannelHints = ConcurrentHashMap.newKeySet(); | ||
|
|
||
| private static Level originalLogLevel; | ||
|
|
||
|
|
@@ -123,8 +117,8 @@ public static void startServer() throws Exception { | |
| server = | ||
| NettyServerBuilder.forAddress(address) | ||
| .addService(mockSpanner) | ||
| // Add a server interceptor to extract channel hints from X-Goog-Spanner-Request-Id | ||
| // header. This verifies that the client uses all configured channels. | ||
| .intercept( | ||
| new ServerInterceptor() { | ||
| @Override | ||
|
|
@@ -138,22 +132,26 @@ public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall( | |
| headers.get( | ||
| Metadata.Key.of( | ||
| "x-response-encoding", Metadata.ASCII_STRING_MARSHALLER))); | ||
| // Extract channel hint from X-Goog-Spanner-Request-Id header | ||
| String requestId = headers.get(XGoogSpannerRequestId.REQUEST_HEADER_KEY); | ||
| if (requestId != null) { | ||
| // Format: | ||
| // <version>.<randProcessId>.<nthClientId>.<nthChannelId>.<nthRequest>.<attempt> | ||
| String[] parts = requestId.split("\\."); | ||
| if (parts.length >= 4) { | ||
| try { | ||
| long channelHint = Long.parseLong(parts[3]); | ||
| if (call.getMethodDescriptor() | ||
| .equals(SpannerGrpc.getBatchCreateSessionsMethod())) { | ||
|
Comment on lines
+144
to
+145
Collaborator
There was a problem hiding this comment. This should probably be replaced with |
||
| batchCreateSessionChannelHints.add(channelHint); | ||
| } | ||
| if (call.getMethodDescriptor() | ||
| .equals(SpannerGrpc.getExecuteStreamingSqlMethod())) { | ||
| executeSqlChannelHints.add(channelHint); | ||
| } | ||
| } catch (NumberFormatException e) { | ||
| // Ignore parse errors | ||
| } | ||
| } | ||
| } | ||
| return Contexts.interceptCall(Context.current(), call, headers, next); | ||
|
|
@@ -185,8 +183,8 @@ public static void resetLogging() { | |
| @After | ||
| public void reset() { | ||
| mockSpanner.reset(); | ||
| batchCreateSessionChannelHints.clear(); | ||
| executeSqlChannelHints.clear(); | ||
| } | ||
|
|
||
| private SpannerOptions createSpannerOptions() { | ||
|
|
@@ -208,34 +206,14 @@ private SpannerOptions createSpannerOptions() { | |
| .build()) | ||
| .setHost("http://" + endpoint) | ||
| .setCredentials(NoCredentials.getInstance()); | ||
|
|
||
| return builder.build(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUsesAllChannels() throws InterruptedException { | ||
| final int multiplier = 2; | ||
| try (Spanner spanner = createSpannerOptions().getService()) { | ||
| DatabaseClient client = spanner.getDatabaseClient(DatabaseId.of("p", "i", "d")); | ||
| ListeningExecutorService executor = | ||
| MoreExecutors.listeningDecorator(Executors.newFixedThreadPool(numChannels * multiplier)); | ||
|
|
@@ -263,13 +241,23 @@ public void testUsesAllChannels() throws InterruptedException { | |
| executor.shutdown(); | ||
| assertTrue(executor.awaitTermination(Duration.ofSeconds(10L))); | ||
| } | ||
| // Bound the channel hints to numChannels (matching gRPC-GCP behavior) and verify | ||
| // that channels are being distributed. The raw channel hints may be unbounded (based on | ||
| // session index), but gRPC-GCP bounds them to the actual number of channels. | ||
| Set<Long> boundedChannelHints = | ||
| executeSqlChannelHints.stream().map(hint -> hint % numChannels).collect(toSet()); | ||
| // Verify that channel distribution is working: | ||
| // - For numChannels=1, exactly 1 channel should be used | ||
| // - For numChannels>1, multiple channels should be used (at least half) | ||
| if (numChannels == 1) { | ||
| assertEquals(1, boundedChannelHints.size()); | ||
| } else { | ||
| assertTrue( | ||
| "Expected at least " | ||
| + (numChannels / 2) | ||
| + " channels to be used, but got " | ||
| + boundedChannelHints.size(), | ||
| boundedChannelHints.size() >= numChannels / 2); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 find this a bit surprising. I would rather have expected this to only be a dependency of
com.google.cloud:google-cloud-spanner(so only the actual hand-written client library).