test(spanner): add a mock server test using key-aware routing#12755
test(spanner): add a mock server test using key-aware routing#12755
Conversation
Adds a mock server test that uses key-aware routing. This test is not testing much interesting, but serves as a base for future tests that can run against a mock server to test for example replica selection.
There was a problem hiding this comment.
Code Review
This pull request modifies MockSpannerServiceImpl to include CacheUpdate in PartialResultSet and removes a restriction on key sizes in isValidKeySet. It also introduces ReplicaSelectionMockServerTest to test end-to-end directed read routing. The review feedback identifies a mismatch in the mock server's statement registration that would cause test failures, and recommends removing an unused field and its redundant cleanup logic.
| com.google.cloud.spanner.Statement readStatement = | ||
| StatementResult.createReadStatement( | ||
| "Table", | ||
| com.google.cloud.spanner.KeySet.singleKey(com.google.cloud.spanner.Key.of()), | ||
| Arrays.asList("Column")); |
There was a problem hiding this comment.
The readStatement is registered with an empty key (Key.of()), but the actual read calls in the polling loop (line 281) use non-empty keys (e.g., "key-1"). In MockSpannerServiceImpl, the registered statement must match the request exactly (including the serialized key set). This mismatch will cause the mock server to return an error as it won't find a matching result. Consider using a consistent fixed key for both registration and the read calls.
| com.google.cloud.spanner.Statement readStatement = | |
| StatementResult.createReadStatement( | |
| "Table", | |
| com.google.cloud.spanner.KeySet.singleKey(com.google.cloud.spanner.Key.of()), | |
| Arrays.asList("Column")); | |
| String key = "key-1"; | |
| com.google.cloud.spanner.Statement readStatement = | |
| StatementResult.createReadStatement( | |
| "Table", | |
| com.google.cloud.spanner.KeySet.singleKey(com.google.cloud.spanner.Key.of(key)), | |
| Arrays.asList("Column")); |
|
|
||
| while (watch.elapsed(TimeUnit.SECONDS) < 10) { | ||
| attempt++; | ||
| String key = "key-" + attempt; |
There was a problem hiding this comment.
To ensure the read request matches the registered result in the mock server, use the same fixed key here as suggested for the registration step.
| String key = "key-" + attempt; | |
| String key = "key-1"; |
|
|
||
| private List<ServerInstance> servers; | ||
| private final int numServers = 2; | ||
| private KeyAwareChannel keyAwareChannel; |
There was a problem hiding this comment.
The field keyAwareChannel is declared but never initialized or used within the test class. It should be removed to maintain code cleanliness.
| if (keyAwareChannel != null) { | ||
| keyAwareChannel.shutdown(); | ||
| } |
There was a problem hiding this comment.
This null check and shutdown call for keyAwareChannel is redundant because the field is never initialized in the current implementation.
Adds a mock server test that uses key-aware routing. This test is not testing much interesting, but serves as a base for future tests that can run against a mock server to test for example replica selection.