FrankChen021 commented on code in PR #19413:
URL: https://github.com/apache/druid/pull/19413#discussion_r3288575318
##########
server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java:
##########
@@ -980,11 +988,32 @@ public Response isHandOffComplete(
Iterable<ImmutableSegmentLoadInfo> servedSegmentsInInterval =
prepareServedSegmentsInInterval(timeline, theInterval);
- if (isSegmentLoaded(servedSegmentsInInterval, descriptor)) {
- return Response.ok(true).build();
+ if (!isSegmentLoaded(servedSegmentsInInterval, descriptor)) {
+ return Response.ok(false).build();
+ }
+
+ // When coordinatingVersions is configured, additionally verify
per-version coverage.
+ final Set<String> coordinatingVersions = coordinatorConfigManager == null
+ ? Set.of()
+ :
coordinatorConfigManager.getCurrentDynamicConfig().getCoordinatingVersions();
+ if (!coordinatingVersions.isEmpty() && matchingLoadRule != null) {
+ final Map<String, Set<String>> activeVersionsByTier =
+ computeActiveVersionsByTier(coordinatingVersions);
+ for (Map.Entry<String, Integer> tierEntry :
matchingLoadRule.getTieredReplicants().entrySet()) {
Review Comment:
[P2] Expand alias tiers before version handoff checks
The coordinator assignment path expands `historicalTierAliases` before
applying load rules, but this handoff check iterates the raw `LoadRule` tiers.
When a rule targets a virtual alias tier such as `hot -> [hot_1, hot_2]`,
`activeVersionsByTier` has entries only for the physical tiers, so the
per-version loop for `hot` is skipped and handoff can return true after the
segment is loaded anywhere, before all required physical tiers and versions
have it. Expand aliases here using the current dynamic config before checking
active versions.
##########
services/src/main/java/org/apache/druid/server/router/TieredBrokerHostSelector.java:
##########
@@ -266,7 +295,26 @@ private Pair<String, Server> getServerPair(String
brokerServiceName)
nodesHolder = servers.get(tierConfig.getDefaultBrokerServiceName());
}
- return new Pair<>(brokerServiceName, nodesHolder.pick());
+ Server picked = nodesHolder.pick();
+ if (picked == null && tierConfig.getRoutableVersions() != null
+ && !tierConfig.getRoutableVersions().isEmpty()) {
+ log.warn(
+ "No brokers available for serviceName[%s] after applying version
filter[%s]. "
+ + "Check that brokers with a matching version are running.",
+ brokerServiceName,
+ tierConfig.getRoutableVersions()
+ );
+ }
+ return new Pair<>(brokerServiceName, picked);
+ }
+
+ private boolean isVersionAllowed(DiscoveryDruidNode node)
+ {
+ final Set<String> routable = tierConfig.getRoutableVersions();
+ if (routable == null || routable.isEmpty()) {
+ return true;
+ }
+ return routable.contains(node.getDruidNode().getVersion());
Review Comment:
[P1] Version filters use the local version after discovery serde
This check relies on the discovered node's announced `DruidNode.version`,
but `DiscoveryDruidNode` is read from ZooKeeper with `DefaultObjectMapper`, and
`DruidNode.version` is a final field that is not accepted by the `@JsonCreator`
while `ALLOW_FINAL_FIELDS_AS_MUTATORS` is disabled. After deserialization,
`getVersion()` is therefore the observer process's package version rather than
the announcing broker's version. In a mixed red/black deployment, routers can
include or exclude brokers based on their own version instead of the broker
version, breaking the new `routableVersions` isolation. Make version a
deserializable creator property and add a discovery serde test with a non-local
version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
