feat: experimental location aware routing- Part 5 #4296
Conversation
This commit adds the foundational SsFormat class that provides sortable string format (ssformat) encoding utilities. This encoding is used by Spanner for key ordering and routing. Key features: - Composite tag encoding for interleaved tables - Signed/unsigned integer encoding (increasing/decreasing) - String and bytes encoding with proper escaping - Double encoding with proper sign handling - Timestamp and UUID encoding - Null value markers with configurable ordering - TargetRange class for key range representation Includes unit tests for all encoding functions. This is part of the experimental location-aware routing for improved latency.
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.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
product-auto-label
bot
added
size: xl
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 a significant new capability for the Google Cloud Spanner client library: key-aware channel integration. This feature enables the client to intelligently route read requests directly to the most appropriate Spanner backend server, bypassing the traditional front-end. This is achieved through a new 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
|
rahul2393
force-pushed
the
feat/key-aware-channel-integration
branch
from
41e3a29 to
e3056a9
Compare
rahul2393
changed the title
![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 a significant new feature for key-aware channel integration, which enables location-aware routing (SpanFE bypass) for Spanner. The changes include a number of new classes for managing channels, caching routing information, and encoding keys. The feature is gated behind an experimental flag and an environment variable.
My review highlights several important points. There are critical issues such as the unimplemented shutdown() and shutdownNow() methods in KeyAwareChannel, which could lead to resource leaks. There's also a high-severity issue regarding the use of a predictable random number generator in production code. Additionally, I've pointed out the extensive use of System.out.println for debugging, which should be replaced with a proper logging framework. I've also included suggestions for code simplification and improving consistency.
| public ManagedChannel shutdownNow() { | ||
| // TODO: Need to manage shutdown of all created channels in serverFactory | ||
| // and clear channelFinders map, potentially shutting down individual finders/channels. | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public ManagedChannel shutdown() { | ||
| // TODO: Need to manage shutdown of all created channels in serverFactory | ||
| return this; | ||
| } |
There was a problem hiding this comment.
The shutdown() and shutdownNow() methods are not implemented and contain TODO comments. A ManagedChannel must properly implement shutdown logic to release resources, such as the underlying channels created by GrpcChannelFinderServerFactory. Without a proper implementation, this can lead to resource leaks. Please implement these methods to properly shut down all channels managed by serverFactory and clean up any other resources.
| } | ||
|
|
||
| // For random value encoding - use seed 12345 for deterministic testing | ||
| private static final java.util.Random testRandom = new java.util.Random(12345); |
There was a problem hiding this comment.
The testRandom field is a java.util.Random instance initialized with a fixed seed, which is then used in encodeRandomValuePart to generate "random" values. Using a predictable random number generator in production code is a security risk. Given its name, this seems intended for testing only. If so, it should be moved to test code or injected as a dependency during tests. If random values are needed in production, a secure random source like java.security.SecureRandom should be used instead.
| String locationApiEnvVar = System.getenv("GOOGLE_SPANNER_EXPERIMENTAL_LOCATION_API"); | ||
| boolean isExperimentalHost = options.isExperimentalHost(); | ||
| boolean envVarEnabled = "true".equalsIgnoreCase(locationApiEnvVar); |
There was a problem hiding this comment.
The logic to check for the GOOGLE_SPANNER_EXPERIMENTAL_LOCATION_API environment variable is implemented directly here using System.getenv(). However, SpannerOptions.java was also modified to add an isEnableLocationApi() method to the SpannerEnvironment interface, which is not being used.
To maintain consistency with how other environment-based flags are handled, you should read this value from SpannerOptions instead. This would involve adding isEnableLocationApi() to SpannerOptions and its builder, and then calling options.isEnableLocationApi() here. This change would centralize environment-dependent configuration within SpannerOptions.
| System.out.println("DEBUG [BYPASS]: Database ID changed from " + databaseId | ||
| + " to " + cacheUpdate.getDatabaseId() + ", clearing caches"); |
There was a problem hiding this comment.
This class, and several other new classes in this PR (KeyAwareChannel, KeyRangeCache, KeyRecipeCache), use System.out.println for logging debug information. In a library, logging should be done through a standard logging framework like java.util.logging (which is already used in GapicSpannerRpc) instead of printing to standard output. This allows consumers of the library to control log levels and output destinations. Please replace these System.out.println calls with appropriate logger calls (e.g., logger.fine(...)).
| synchronized (channelBuilder) { | ||
| InstantiatingGrpcChannelProvider.Builder newBuilder = | ||
| channelBuilder.setEndpoint(addr); | ||
| return new GrpcChannelFinderServer(addr, newBuilder.build()); | ||
| } |
There was a problem hiding this comment.
The synchronized (channelBuilder) block within the computeIfAbsent lambda appears to be unnecessary and could be confusing. computeIfAbsent is already atomic for a given key. Inside the lambda, channelBuilder.setEndpoint(addr) returns a new builder instance rather than mutating the shared channelBuilder field, so there is no shared mutable state being modified that would require synchronization. Removing the synchronized block would make the code clearer.
InstantiatingGrpcChannelProvider.Builder newBuilder =
channelBuilder.setEndpoint(addr);
return new GrpcChannelFinderServer(addr, newBuilder.build());| private static long[] parseTimestamp(String ts) { | ||
| // Parse RFC3339 timestamp using Java time library | ||
| // Remove trailing Z and parse | ||
| String withoutZ = ts.substring(0, ts.length() - 1); | ||
|
|
||
| // Parse date-time parts | ||
| int dotIdx = withoutZ.indexOf('.'); | ||
| String dateTimePart; | ||
| int nanos = 0; | ||
| if (dotIdx >= 0) { | ||
| dateTimePart = withoutZ.substring(0, dotIdx); | ||
| String fracStr = withoutZ.substring(dotIdx + 1); | ||
| // Pad to 9 digits | ||
| while (fracStr.length() < 9) { | ||
| fracStr = fracStr + "0"; | ||
| } | ||
| // Truncate to 9 digits | ||
| if (fracStr.length() > 9) { | ||
| fracStr = fracStr.substring(0, 9); | ||
| } | ||
| nanos = Integer.parseInt(fracStr); | ||
| } else { | ||
| dateTimePart = withoutZ; | ||
| } | ||
|
|
||
| // Parse date and time components | ||
| // Format: YYYY-MM-DDTHH:MM:SS | ||
| String[] dateTime = dateTimePart.split("T"); | ||
| String[] dateParts = dateTime[0].split("-"); | ||
| String[] timeParts = dateTime[1].split(":"); | ||
|
|
||
| int year = Integer.parseInt(dateParts[0]); | ||
| int month = Integer.parseInt(dateParts[1]); | ||
| int day = Integer.parseInt(dateParts[2]); | ||
| int hour = Integer.parseInt(timeParts[0]); | ||
| int minute = Integer.parseInt(timeParts[1]); | ||
| int second = Integer.parseInt(timeParts[2]); | ||
|
|
||
| // Compute days since epoch using proleptic Gregorian calendar | ||
| long days = civilDayNumber(year, month, day); | ||
| long seconds = days * 86400L + hour * 3600L + minute * 60L + second; | ||
|
|
||
| return new long[] {seconds, nanos}; | ||
| } | ||
|
|
||
| // Compute the civil day number (days since Unix epoch 1970-01-01) | ||
| // This matches absl::CivilDay calculation | ||
| private static long civilDayNumber(int year, int month, int day) { | ||
| // Algorithm from http://howardhinnant.github.io/date_algorithms.html | ||
| // This produces the same results as absl::CivilDay | ||
| int y = year; | ||
| int m = month; | ||
| int d = day; | ||
|
|
||
| // Adjust year and month (March = month 1 in this algorithm) | ||
| if (m <= 2) { | ||
| y -= 1; | ||
| m += 12; | ||
| } | ||
| m -= 3; | ||
|
|
||
| // Days from era 0 (year 0 March 1) to given date | ||
| int era = (y >= 0 ? y : y - 399) / 400; | ||
| int yoe = y - era * 400; // year of era [0, 399] | ||
| int doy = (153 * m + 2) / 5 + d - 1; // day of year [0, 365] | ||
| int doe = yoe * 365 + yoe / 4 - yoe / 100 + doy; // day of era [0, 146096] | ||
| long dayNumber = | ||
| (long) era * 146097 + doe - 719468; // shift epoch from 0000-03-01 to 1970-01-01 | ||
|
|
||
| return dayNumber; | ||
| } |
There was a problem hiding this comment.
The methods parseTimestamp and civilDayNumber (used by parseDate) implement custom logic for parsing RFC3339 timestamps and dates. The java.time API, available since Java 8, provides robust and standard ways to handle this which would make the code simpler and less error-prone.
For example:
parseTimestampcould be simplified usingjava.time.Instant.parse().parseDatecould usejava.time.LocalDate.parse(dateStr).toEpochDay()to get the number of days since the epoch.
Consider refactoring to use the standard library unless there's a specific performance or compatibility reason for this custom implementation.
| * <li>A database with table T created and populated | ||
| * </ul> | ||
| */ | ||
| public class BypassPointReadTest { |
There was a problem hiding this comment.
This test is implemented as a class with a main method, which is not standard for tests in a Maven project. It should be converted into a JUnit test (e.g., using @Test annotations). This would allow it to be run as part of the standard build and test cycle. Using System.exit(1) is also not ideal for a test suite, as it can halt the entire test run. A JUnit test would fail gracefully and report the error without exiting.
This commit adds the key recipe system for translating requests to sortable string format (ssformat) keys for routing. Key components: - KeyRecipe: Translates requests to ssformat keys using server-provided recipes - KeyRecipeCache: Caches recipes by request fingerprint for efficient reuse - PreparedRead: Helper class for preparing read requests with routing info - RecipeGoldenTest: Golden tests for recipe encoding validation - RecipeTestCases: Test case definitions for recipe tests This is part of the experimental location-aware routing for improved latency.
This commit adds the KeyRangeCache class that maps key ranges to specific server locations for routing decisions. Key features: - TabletEntry class for tablet metadata (UID, server address, incarnation) - ServerEntry class for server connection management - Key range to tablet mapping with efficient lookup - Lazy server initialization for on-demand connections - Integration with ChannelFinderServer interfaces This is part of the experimental location-aware routing for improved latency.
This commit adds the KeyAwareChannel class and integrates all components for location-aware routing in the Spanner client. Key components: - ChannelFinder: Orchestrates routing decisions using caches - KeyAwareChannel: Custom ManagedChannel that intercepts key-aware methods - SpannerOptions integration: Enables feature via setExperimentalHost() - GapicSpannerRpc modifications: Wire in KeyAwareChannel Activation requires: - SpannerOptions.Builder.setExperimentalHost() with location-aware endpoint - GOOGLE_SPANNER_EXPERIMENTAL_LOCATION_API=true environment variable This is part of the experimental location-aware routing for improved latency.
rahul2393
force-pushed
the
feat/key-aware-channel-integration
branch
from
4d74d9e to
5b5e6f5
Compare
2 participants
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.