feat: add ChannelFinder server interfaces #4293
Conversation
This commit adds the server abstraction interfaces for location-aware routing: - ChannelFinderServer: Interface representing a Spanner server endpoint with address, health check, and channel access - ChannelFinderServerFactory: Factory interface for creating and caching server connections - GrpcChannelFinderServerFactory: gRPC implementation that creates and manages gRPC channels for different server endpoints These interfaces enable the client to maintain connections to multiple Spanner servers and route requests directly to the appropriate server based on key location information. This is part of the experimental location-aware routing for improved latency.
product-auto-label
bot
added
size: m
Summary of ChangesHello @rahul2393, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces foundational interfaces and a gRPC-specific implementation to support an experimental location-aware routing feature within the Spanner client library. The new components enable the client to establish and manage connections to multiple Spanner server endpoints, facilitating intelligent request routing based on key location information. This enhancement is designed to improve overall latency by directing requests to the most geographically appropriate server. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 new interfaces and an implementation for location-aware routing, which is a great step towards improving latency. The new abstractions ChannelFinderServer and ChannelFinderServerFactory are well-defined. My review includes suggestions for improving the Javadoc for these new interfaces to make them clearer. I've also identified a potential concurrency issue in GrpcChannelFinderServerFactory where a shared builder is mutated, and I've proposed a fix. Additionally, there are some minor points on error handling and code clarity.
| synchronized (channelBuilder) { | ||
| InstantiatingGrpcChannelProvider.Builder newBuilder = | ||
| channelBuilder.setEndpoint(addr); | ||
| return new GrpcChannelFinderServer(addr, newBuilder.build()); | ||
| } |
There was a problem hiding this comment.
The channelBuilder is being mutated here without restoring its original state. This is a shared object, and modifying it can lead to race conditions and unpredictable behavior if other threads use it. The synchronized block prevents concurrent modification, but the state change persists. You should restore the original endpoint of the builder in a finally block to ensure the builder's state is not permanently altered.
synchronized (channelBuilder) {
String originalEndpoint = channelBuilder.getEndpoint();
try {
InstantiatingGrpcChannelProvider.Builder newBuilder =
channelBuilder.setEndpoint(addr);
return new GrpcChannelFinderServer(addr, newBuilder.build());
} finally {
channelBuilder.setEndpoint(originalEndpoint);
}
}| String getAddress(); | ||
|
|
||
| boolean isHealthy(); | ||
|
|
||
| ManagedChannel getChannel(); // Added to get the underlying channel for RPC calls |
There was a problem hiding this comment.
The methods in this interface should have Javadoc comments to explain their purpose. The inline comment on getChannel should also be converted to a Javadoc comment for consistency and clarity.
/** Returns the address of this server endpoint. */
String getAddress();
/** Returns true if the server is considered healthy and can accept requests. */
boolean isHealthy();
/** Returns the underlying channel for making RPC calls to this server. */
ManagedChannel getChannel();| ChannelFinderServer defaultServer(); | ||
|
|
||
| ChannelFinderServer create(String address); |
There was a problem hiding this comment.
The methods in this interface should have Javadoc comments to explain their purpose, expected inputs, and what they return. This will improve the maintainability and usability of this new API.
/** Returns the default server that is used if no specific server can be found for a request. */
ChannelFinderServer defaultServer();
/**
* Creates or returns a cached server for the given address.
*
* @param address The address of the server to connect to.
* @return A {@link ChannelFinderServer} for the given address.
*/
ChannelFinderServer create(String address);| return new GrpcChannelFinderServer(addr, newBuilder.build()); | ||
| } | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Failed to create channel for address: " + addr, e); |
There was a problem hiding this comment.
Wrapping the IOException in a generic RuntimeException is not ideal. It would be more consistent with the rest of the codebase to use SpannerExceptionFactory to create a more specific SpannerException. This provides better error handling and diagnostics.
throw SpannerExceptionFactory.newSpannerException(ErrorCode.UNAVAILABLE, "Failed to create channel for address: " + addr, e);| // Constructor for the default server that already has a channel | ||
| public GrpcChannelFinderServer(String address, ManagedChannel channel) { | ||
| this.address = address; | ||
| this.channel = channel; | ||
| } |
There was a problem hiding this comment.
This constructor GrpcChannelFinderServer(String address, ManagedChannel channel) appears to be unused in GrpcChannelFinderServerFactory. The comment above it is also misleading as the default server is created using the other constructor. If this constructor is not needed, it should be removed to avoid dead code. If it's intended for testing, it should be annotated with @VisibleForTesting.
1 participant
This commit adds the server abstraction interfaces for location-aware routing:
These interfaces enable the client to maintain connections to multiple Spanner servers and route requests directly to the appropriate server based on key location information.
This is part of the experimental location-aware routing for improved latency.