Jackie-Jiang commented on code in PR #19891:
URL: https://github.com/apache/pinot/pull/19891#discussion_r4300442487
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -577,16 +595,22 @@ public SelectionResult select(BrokerRequest
brokerRequest, List<String> segments
SegmentStates segmentStates = _segmentStates;
InstanceMapping mapping = select(segments, requestIdInt, segmentStates,
queryOptions);
Set<String> unavailableSegments = segmentStates.getUnavailableSegments();
+ Set<String> segmentsWithoutCandidates =
segmentStates.getSegmentsWithoutCandidates();
if (unavailableSegments.isEmpty()) {
return new SelectionResult(mapping, List.of(), 0);
} else {
List<String> unavailableSegmentsForRequest = new ArrayList<>();
+ boolean failOpenEligible = false;
for (String segment : segments) {
if (unavailableSegments.contains(segment)) {
unavailableSegmentsForRequest.add(segment);
+ failOpenEligible |= !segmentsWithoutCandidates.contains(segment);
}
}
+ if (failOpenEligible) {
+
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.FAIL_OPEN_ELIGIBLE_REQUESTS,
1);
Review Comment:
[P2] Count eligible requests before subclass selection can throw.
The protected `select(...)` call at line 596 runs before this increment. For
`multiStageReplicaGroup`, an old segment whose ONLINE IS/EV candidates are all
excluded has no enabled candidate list, and
`MultiStageReplicaGroupSelector.assign()` throws before reaching this meter.
The segment meter increments, but affected requests are omitted. Classify and
record eligibility from the captured snapshot before invoking subclass
selection, preserving the existing exception behavior. The new test's
`selectorType` provider does not include this selector.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -446,6 +447,9 @@ void refreshSegmentStates() {
+ "(all candidate instances: {} are disabled, counting segment
as unavailable)", segment,
_tableNameWithType, candidateInstances);
unavailableSegments.add(segment);
+ if (candidates.isEmpty()) {
+ segmentsWithoutCandidates.add(segment);
Review Comment:
[P2] Distinguish failure-detector exclusions from administrative disables.
A nonempty candidate list does not establish the eligibility described in
the PR's testing section. The supplied enabled-instance set excludes servers
with `HELIX_ENABLED=false`, `QUERIES_DISABLED=true`, or shutdown in progress,
as well as failure-detector exclusions. If all replicas are administratively
disabled while IS/EV remain ONLINE, both new meters count them as fail-open
eligible. Please carry the exclusion reason into this classification and
require at least one candidate that is still administratively enabled but
excluded by the failure detector. A routing-manager test contrasting these
cases would cover the distinction.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/SegmentStates.java:
##########
@@ -38,12 +38,22 @@ public class SegmentStates {
private final Map<String, List<SegmentInstanceCandidate>>
_instanceCandidatesMap;
private final Set<String> _servingInstances;
private final Set<String> _unavailableSegments;
+ private final Set<String> _segmentsWithoutCandidates;
Review Comment:
Simplification: store `_failOpenEligibleSegments` directly here. Both
consumers currently reconstruct eligibility as unavailable minus
candidate-less, which introduces compound conditions and negation. An explicit
eligible set lets transition detection use
`!previousEligibleSegments.contains(segment)` and request classification use
`eligibleSegments.contains(segment)`. The existing three-argument constructor
can default it to `Set.of()`, preserving its conservative behavior. Keep the
set in this same published snapshot so routing and eligibility stay consistent.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -470,7 +474,21 @@ void refreshSegmentStates() {
}
}
- _segmentStates = new SegmentStates(instanceCandidatesMap,
servingInstances, unavailableSegments);
+ SegmentStates previousSegmentStates = _segmentStates;
+ Set<String> previousUnavailableSegments =
+ previousSegmentStates != null ?
previousSegmentStates.getUnavailableSegments() : Set.of();
+ Set<String> previousSegmentsWithoutCandidates =
+ previousSegmentStates != null ?
previousSegmentStates.getSegmentsWithoutCandidates() : Set.of();
+ for (String segment : unavailableSegments) {
+ boolean wasAlreadyUnavailableWithCandidates =
+ previousUnavailableSegments.contains(segment) &&
!previousSegmentsWithoutCandidates.contains(segment);
+ if (!segmentsWithoutCandidates.contains(segment) &&
!wasAlreadyUnavailableWithCandidates) {
+ _brokerMetrics.addMeteredTableValue(_tableNameWithType,
BrokerMeter.FAIL_OPEN_ELIGIBLE_SEGMENTS, 1);
Review Comment:
[P2] Emit segment transitions only from the main table selector.
`BaseBrokerRoutingManager` creates a separate selector for each configured
table sampler, using the same table name and `BrokerMetrics`, and updates all
of them on instance changes. A segment included in N samplers therefore
increments this meter N+1 times for one transition. The main routing entry
should own table-level emission, following the existing replica-health
reporting approach. An overlapping-sampler test would catch this. The
transition count can also be accumulated during the existing old-segment loop
and emitted once, avoiding the extra pass and repeated meter calls.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -577,16 +595,22 @@ public SelectionResult select(BrokerRequest
brokerRequest, List<String> segments
SegmentStates segmentStates = _segmentStates;
InstanceMapping mapping = select(segments, requestIdInt, segmentStates,
queryOptions);
Set<String> unavailableSegments = segmentStates.getUnavailableSegments();
+ Set<String> segmentsWithoutCandidates =
segmentStates.getSegmentsWithoutCandidates();
if (unavailableSegments.isEmpty()) {
return new SelectionResult(mapping, List.of(), 0);
} else {
List<String> unavailableSegmentsForRequest = new ArrayList<>();
+ boolean failOpenEligible = false;
for (String segment : segments) {
if (unavailableSegments.contains(segment)) {
unavailableSegmentsForRequest.add(segment);
+ failOpenEligible |= !segmentsWithoutCandidates.contains(segment);
}
}
+ if (failOpenEligible) {
+
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.FAIL_OPEN_ELIGIBLE_REQUESTS,
1);
Review Comment:
[P2] Deduplicate request counts across scans of the same physical table.
This increments once per `select` invocation, while the meter documents once
per query per physical table. Both MSE planners route separate leaf scans
independently. A non-partitioned self-join with distinct leaves that both touch
an eligible segment can therefore increment twice for the same request and
physical table. Aggregate eligibility in request-scoped state and emit once per
physical table; a self-join regression with distinct leaf filters would cover
this.
--
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]
