chore(spanner): vendor grpc-gcp sources into google-cloud-spanner#12745
chore(spanner): vendor grpc-gcp sources into google-cloud-spanner#12745
Conversation
rahul2393
force-pushed
the
vendor-grpc-gcp
branch
from
931907a to
c0545fc
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| private final ExecutorService stateNotificationExecutor = | ||
| Executors.newCachedThreadPool( | ||
| GcpThreadFactory.newThreadFactory("gcp-mc-state-notifications-%d")); |
There was a problem hiding this comment.
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
- Use bounded thread pools to prevent resource exhaustion. (link)
- 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.
|
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. |
|
@suztomo You're right. I switched this from vendoring classes into |
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> |
There was a problem hiding this comment.
This line should be managed by Release Please.
I believe there's one in sdk-platform-java/java-shared-dependencies.
|
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) |
2 participants
Summary
This change vendors the
grpc-gcplibrary directly intogoogle-cloud-spannerso localgrpc-gcpchanges can be used injava-spannerwithout publishing a Maven snapshot first.What changed
grpc-gcpmain sources intogoogle-cloud-spannercom.google.cloud.grpc.proto.*AutoValue_OpenTelemetryMetricsResourcegrpc-gcpunit tests and test resources intogoogle-cloud-spannercom.google.cloud:grpc-gcpdependency fromgoogle-cloud-spannerthird-party/grpc-gcpstaging README1.9.3-SNAPSHOTversion string with a derived local implementation versionWhy
Today, making
grpc-gcpchanges available tojava-spannerrequires:grpc-gcpsdk-platform-javajava-spannerThat slows down iteration. Vendoring the library into
google-cloud-spannermakes local development and testing ofgrpc-gcpchanges possible in a single repo without relying on a published Maven artifact.