xiangfu0 commented on code in PR #19891:
URL: https://github.com/apache/pinot/pull/19891#discussion_r4509129569
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -575,20 +592,25 @@ public SelectionResult select(BrokerRequest
brokerRequest, List<String> segments
// Copy the volatile reference so that the segment-to-instance map and
unavailable segments have a consistent view
// of the state.
SegmentStates segmentStates = _segmentStates;
- InstanceMapping mapping = select(segments, requestIdInt, segmentStates,
queryOptions);
Set<String> unavailableSegments = segmentStates.getUnavailableSegments();
-
- if (unavailableSegments.isEmpty()) {
- return new SelectionResult(mapping, List.of(), 0);
- } else {
- List<String> unavailableSegmentsForRequest = new ArrayList<>();
+ List<String> unavailableSegmentsForRequest = List.of();
+ // Classify before subclass selection, which can throw on unavailable
segments (e.g. multi-stage replica-group)
Review Comment:
This also conflicts with master. `select(BrokerRequest, ...)` there just
delegates to `select(segments, requestIdInt, _segmentStates, queryOptions)`,
and the unavailable-segment loop and `getUnavailableSegments()` set are gone
(unavailability is now per group id). The request classification needs to move
into the group-based path, and still has to run before subclass selection,
which can throw.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -414,6 +418,11 @@ void refreshSegmentStates() {
new
HashMap<>(HashUtil.getHashMapCapacity(_oldSegmentCandidatesMap.size() +
_newSegmentStateMap.size()));
Set<String> servingInstances = new HashSet<>();
Set<String> unavailableSegments = new HashSet<>();
+ Set<String> failOpenEligibleSegments = new HashSet<>();
Review Comment:
This conflicts with #20077 (now on master). `refreshSegmentStates` there
works on candidate groups, and its doc says the cost depends on the number of
groups and instances, not segments. A per-segment `HashSet` plus a `contains`
on every refresh would bring that O(segments) cost back for every table on each
instance change. A group-level version keeps the invariant: a group is eligible
when `group._routing` has candidates but the filtered routing has none. Count
by `groupSize`, and detect transitions with the existing
`previousEnabledBySlot` when the `SegmentGroups` are unchanged.
--
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]
