xiangfu0 commented on code in PR #19537:
URL: https://github.com/apache/pinot/pull/19537#discussion_r4221912592
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -933,6 +939,11 @@ private void
assignWorkersToNonPartitionedLeafFragment(PlanFragment fragment, Di
context.addNumSegmentsPrunedByBroker(routingTable.getNumPrunedSegments());
}
}
+ if (serverInstanceToSegmentsMap.isEmpty()) {
Review Comment:
**This disables the all-leaf-stages-empty short-circuit for the
non-partitioned path.** Previously an empty `serverInstanceToSegmentsMap` left
`workerIdToServerInstanceMap` empty, `updateContextForLeafStage` called
`recordLeafStageEmpty()`, and `MultiStageBrokerRequestHandler` ran only the
broker reduce stage with no server dispatch. The fallback now always assigns
one worker, so fully-pruned or empty-table queries dispatch a useless server
round-trip. If the intent is only to fix the mixed case (one empty leaf joined
with non-empty leaves), consider keeping the zero-worker assignment when the
whole plan can short-circuit, or recording the stage as empty despite the
placeholder worker.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -2075,4 +2108,35 @@ private static Map<String, List<String>>
getSegmentsMap(@Nullable List<String> o
return Map.of(TableType.REALTIME.name(), realtimeSegments);
}
}
+
+ private void
assignWorkersForNonPartitionedLeafSegmentsWhenNoServersHaveSegments(DispatchablePlanMetadata
metadata,
+ String tableName, Map<String, RoutingTable> routingTableMap,
DispatchablePlanContext context,
+ @Nullable TimeBoundaryInfo timeBoundaryInfo) {
+ Set<String> serversRoutingToTable =
_routingManager.getServingInstances(tableName);
+ if (CollectionUtils.isEmpty(serversRoutingToTable)) {
+ LOGGER.error("[RequestId: {}] No routable server for empty routing on
table: {}", context.getRequestId(),
+ tableName);
+ throw new IllegalStateException("No routable server for empty routing on
table: " + tableName);
+ }
+
+ Map<String, ServerInstance> serverInstanceMap =
_routingManager.getRoutableServerInstanceMap();
+ ServerInstance serverInstance =
serverInstanceMap.get(serversRoutingToTable.iterator().next());
Review Comment:
**Possible NPE.** `serverInstanceMap.get(...)` can return `null` when the
picked serving instance is not in the routable map (e.g. excluded by the
FailureDetector or draining), and `new QueryServerInstance(null)` then NPEs.
Also `serversRoutingToTable.iterator().next()` picks an arbitrary instance
rather than the first one that is actually routable — better to iterate
`serversRoutingToTable` and take the first id present in `serverInstanceMap`,
failing with the explicit error above if none is.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -1109,9 +1128,6 @@ private PinotQuery extractRoutingQuery(PlanNode
leafStageRoot, String tableName,
if (!useBrokerPruning) {
return null;
}
- if (!PlanNodeRoutingQueryBuilder.canBuildRoutingQuery(leafStageRoot)) {
- return null;
- }
try {
Review Comment:
**Removing the `canBuildRoutingQuery` guard reintroduces a per-query
exception.** The guard's own doc says it exists so a colocated semi-join's
probe leaf (which holds a multi-input join node) does not pay a
thrown-and-caught `IllegalStateException` from `accumulateBottomToTop` on every
query. With it removed, every such query now throws, is caught here, and logs a
WARN — steady-state log spam plus exception overhead on the planning hot path.
`canBuildRoutingQuery` also becomes unused dead code. If the removal is
intentional, the WARN should at least be demoted/debounced and the dead method
deleted.
--
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]
