DBZ-8053 Handle empty shards #202
Conversation
|
@jpechane can you take a look when you get the chance? thanks! |
|
@twthorn Hi, I was OOO. Could you please rebase the PR on the latest main? Thanks |
| /** | ||
| * Class for getting metadata on Vitess, e.g., tables, shards. Supports shard-specific queries. | ||
| */ | ||
| public class VitessMetadata { |
There was a problem hiding this comment.
Would it be possible to convert this class to be no-static? A lot of methods uses VitessConnectorConfig so maybe an instance can be created that will receive the config in constructor.
|
@jpechane this is ready for re-review, thanks! |
| @@ -75,6 +76,21 @@ public Vtgate.ExecuteResponse execute(String sqlStatement) { | |||
| return newBlockingStub(channel).execute(request); | |||
| } | |||
|
|
|||
| public Vtgate.ExecuteResponse execute(String sqlStatement, String shard) { | |||
| LOGGER.info("Executing sqlStament {}", sqlStatement); | |||
There was a problem hiding this comment.
| LOGGER.info("Executing sqlStament {}", sqlStatement); | |
| LOGGER.debug("Executing sqlStament {}", sqlStatement); |
|
|
||
| String target = String.format("%s:%s@%s", config.getKeyspace(), shard, config.getTabletType()); | ||
| Vtgate.Session session = Vtgate.Session.newBuilder().setTargetString(target).setAutocommit(true).build(); | ||
| LOGGER.info("Autocommit {}", session.getAutocommit()); |
There was a problem hiding this comment.
| LOGGER.info("Autocommit {}", session.getAutocommit()); | |
| LOGGER.debug("Autocommit {}", session.getAutocommit()); |
| @@ -1025,6 +1028,152 @@ public void shouldUseSameTransactionIdWhenMultiGrpcResponses() throws Exception | |||
| Testing.Print.enable(); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Would it be possible to add a test to verify the behaviour when EXCLUDE_EMPTY_SHARDS is not set?
There was a problem hiding this comment.
If EXCLUDE_EMPTY_SHARDS is not set, and we try to stream from the keyspace with an empty shard, it fails and brings down the test vitess instance, so subsequent unrelated tests would also fail. All other tests in the integration tests do not set this variable and it defaults to false so they are effectively testing it without EXCLUDE_EMPTY_SHARDS being set.
| } | ||
|
|
||
| private static void logResponse(Vtgate.ExecuteResponse response, String query) { | ||
| LOGGER.info("Got response: {} for query: {}", response, query); |
There was a problem hiding this comment.
| LOGGER.info("Got response: {} for query: {}", response, query); | |
| LOGGER.debug("Got response: {} for query: {}", response, query); |
| } | ||
| } | ||
|
|
||
| private static List<String> getFlattenedRowsFromResponse(Vtgate.ExecuteResponse response) { |
There was a problem hiding this comment.
Is it possible to make the rest of the methods non-static too?
There was a problem hiding this comment.
It is possible, but I don't think we want to, since they don't rely on any instance variables. Static clearly conveys these methods do not rely on object state.
|
@twthorn Applied, thanks! |
DBZ-8053
In some cases, a Vitess keyspace can have empty shards (shards without tablets). Do the following to handle this: