Skip to content

chore(spanner): vendor grpc-gcp sources into google-cloud-spanner#12745

Open
rahul2393 wants to merge 4 commits intomainfrom
vendor-grpc-gcp
Open

chore(spanner): vendor grpc-gcp sources into google-cloud-spanner#12745
rahul2393 wants to merge 4 commits intomainfrom
vendor-grpc-gcp

Conversation

@rahul2393
Copy link
Copy Markdown
Contributor

Summary

This change vendors the grpc-gcp library directly into google-cloud-spanner so local grpc-gcp changes can be used in java-spanner without publishing a Maven snapshot first.

What changed

  • copied grpc-gcp main sources into google-cloud-spanner
  • copied required generated sources:
    • protobuf-generated com.google.cloud.grpc.proto.*
    • generated AutoValue_OpenTelemetryMetricsResource
  • copied grpc-gcp unit tests and test resources into google-cloud-spanner
  • skipped the upstream integration tests that depend on external Spanner/Bigtable setup
  • removed the external com.google.cloud:grpc-gcp dependency from google-cloud-spanner
  • added the remaining compile-time annotation dependency needed by the vendored sources
  • removed the obsolete third-party/grpc-gcp staging README
  • replaced the hardcoded 1.9.3-SNAPSHOT version string with a derived local implementation version

Why

Today, making grpc-gcp changes available to java-spanner requires:

  1. releasing grpc-gcp
  2. updating dependencies in sdk-platform-java
  3. then consuming the updated artifact from java-spanner

That slows down iteration. Vendoring the library into google-cloud-spanner makes local development and testing of grpc-gcp changes possible in a single repo without relying on a published Maven artifact.

@rahul2393 rahul2393 requested review from a team as code owners April 10, 2026 13:57
@rahul2393 rahul2393 requested a review from olavloite April 10, 2026 13:59
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces significant changes to the gRPC-GCP library, including the addition of GcpClientCall, GcpManagedChannel, GcpMultiEndpointChannel, and related configuration and utility classes to support advanced channel management, affinity, and multi-endpoint routing. It also updates the project dependencies in pom.xml. My feedback highlights concerns regarding the use of an unbounded thread pool for state notifications, which could lead to resource exhaustion, and suggests explicitly managing the version of the newly added auto-value-annotations dependency to ensure build reproducibility.

Comment on lines +147 to +149
private final ExecutorService stateNotificationExecutor =
Executors.newCachedThreadPool(
GcpThreadFactory.newThreadFactory("gcp-mc-state-notifications-%d"));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Using Executors.newCachedThreadPool() for state notifications can lead to resource exhaustion under high load. Consider using a bounded thread pool or a shared executor service.

References
  1. Use bounded thread pools to prevent resource exhaustion. (link)
  2. For safety, use a bounded thread pool (e.g., ThreadPoolExecutor with a defined maximum size) instead of an unbounded one (e.g., Executors.newCachedThreadPool()), even if the current logic seems to limit concurrent tasks.

@suztomo
Copy link
Copy Markdown
Member

suztomo commented Apr 10, 2026

It seems you're copying the code from grpc-gcp to google-cloud-spanner. I'm afraid this will cause many NoSuchMethodError or NoClassDefFoundError going forward. https://jlbp.dev/JLBP-5 "Do not include a class in more than one classpath entry"

The right way is create grpc-gcp module with the same group ID and artifact ID as before. Maven and Gradle ensure that only one version of grpc-gcp artifact exists in the class path.

@rahul2393
Copy link
Copy Markdown
Contributor Author

@suztomo You're right. I switched this from vendoring classes into google-cloud-spanner to a local grpc-gcp module with the same com.google.cloud:grpc-gcp coordinates, and restored google-cloud-spanner to depend on that artifact. That avoids duplicate-classpath issues while still allowing local iteration.

<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<github.global.server>github</github.global.server>
<site.installationModule>google-cloud-spanner-parent</site.installationModule>
<grpc-gcp.version>1.9.3-SNAPSHOT</grpc-gcp.version>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line should be managed by Release Please.

I believe there's one in sdk-platform-java/java-shared-dependencies.

@rahul2393 rahul2393 requested a review from suztomo April 10, 2026 14:52
@suztomo
Copy link
Copy Markdown
Member

suztomo commented Apr 10, 2026

Let me merge #12746 first to make a releasable source tree and avoid unexpected unknown problem due to grpc-gcp. (com.google.cloud:grpc-gcp:1.9.2 already exists in Maven Central)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants