FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

chore(spanner): vendor grpc-gcp sources into google-cloud-spanner by rahul2393 · Pull Request #12745 · googleapis/google-cloud-java · GitHub

/ google-cloud-java Public

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

Closed
rahul2393 wants to merge 5 commits into
mainfrom
vendor-grpc-gcp
ConversationCommits (5)ChecksFiles changed
Closed

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

Conversation

rahul2393 commented Apr 10, 2026

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 requested review from a team as code owners April 10, 2026 13:57
rahul2393 requested a review from olavloite April 10, 2026 13:59
gemini-code-assist Bot reviewed Apr 10, 2026
View reviewed changes

gemini-code-assist Bot left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 thread java-spanner/grpc-gcp/src/main/java/com/google/cloud/grpc/GcpManagedChannel.java
Comment on lines +147 to +149
private final ExecutorService stateNotificationExecutor =
Executors.newCachedThreadPool(
GcpThreadFactory.newThreadFactory("gcp-mc-state-notifications-%d"));

gemini-code-assist Bot Apr 10, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread java-spanner/google-cloud-spanner/pom.xml Outdated

suztomo commented Apr 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Apr 10, 2026

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.

suztomo reviewed Apr 10, 2026
View reviewed changes
Comment thread java-spanner/grpc-gcp/pom.xml Outdated
suztomo reviewed Apr 10, 2026
View reviewed changes
Comment thread java-spanner/pom.xml Outdated
<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>

suztomo Apr 10, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This line should be managed by Release Please.

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

rahul2393 requested a review from suztomo April 10, 2026 14:52

suztomo commented Apr 10, 2026

Copy link
Copy Markdown
Member

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)

suztomo commented Apr 13, 2026

Copy link
Copy Markdown
Member

There are actually more things you need to consider, such as importing Git history and setting up CIs. Mike is the best contact.

@meltsufin Would you help Rahul about how they release grpc-gcp (currently at https://github.com/GoogleCloudPlatform/grpc-gcp-java) going forward?

meltsufin commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Are you trying to migrate the grpc-gcp repo to the monorepo here?

rahul2393 commented Apr 14, 2026

Copy link
Copy Markdown
Contributor Author

@meltsufin yes please help us to complete migration.

meltsufin commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

@meltsufin yes please help us to complete migration.

I responded by email.

rahul2393 commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

Closing this in favor of #13184

rahul2393 closed this May 13, 2026
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

olavloite Awaiting requested review from olavloite
suztomo Awaiting requested review from suztomo
1 more reviewer
gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL