raghavyadav01 commented on code in PR #19891:
URL: https://github.com/apache/pinot/pull/19891#discussion_r4505327679
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -446,6 +455,13 @@ void refreshSegmentStates() {
+ "(all candidate instances: {} are disabled, counting segment
as unavailable)", segment,
_tableNameWithType, candidateInstances);
unavailableSegments.add(segment);
+ if (!candidates.isEmpty()) {
+ failOpenEligibleSegments.add(segment);
Review Comment:
This marks a segment fail-open eligible when its candidates are
Helix-disabled or shutting down, not only FailureDetector-excluded. #19892 says
disabled servers are never used for fail-open. Should eligibility count only
FailureDetector-excluded candidates, so the metric doesn't spike on every
rolling restart?
##########
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)
+ if (!unavailableSegments.isEmpty()) {
+ Set<String> failOpenEligibleSegments =
segmentStates.getFailOpenEligibleSegments();
+ unavailableSegmentsForRequest = new ArrayList<>();
+ boolean failOpenEligible = false;
for (String segment : segments) {
if (unavailableSegments.contains(segment)) {
unavailableSegmentsForRequest.add(segment);
+ failOpenEligible |= failOpenEligibleSegments.contains(segment);
}
}
- return new SelectionResult(mapping, unavailableSegmentsForRequest, 0);
+ if (failOpenEligible &&
_lastFailOpenEligibleRequestId.getAndSet(requestId) != requestId) {
Review Comment:
A hybrid-table query goes through both the OFFLINE and REALTIME selectors,
so this global meter is bumped twice for one query. Should it be counted once
per broker request instead?
--
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]
