xiangfu0 commented on code in PR #19891:
URL: https://github.com/apache/pinot/pull/19891#discussion_r4288465265
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -216,6 +216,14 @@ public class BrokerMeter implements AbstractMetrics.Meter {
public static final BrokerMeter ROUTING_TABLE_UPDATE_FAILURE =
create("ROUTING_TABLE_UPDATE_FAILURE", "failures", false);
+ // Panic routing metrics
+ public static final BrokerMeter FAIL_OPEN_ELIGIBLE_REQUESTS =
+ create("FAIL_OPEN_ELIGIBLE_REQUESTS", "failOpenEligibleRequests", true);
+
+ // Tracks segments that become eligible for fail-open routing.
+ public static final BrokerMeter FAIL_OPEN_ELIGIBLE_SEGMENTS =
Review Comment:
Minor: most recently added meters here carry a `///` Javadoc explaining
exactly when they are emitted. These two have non-obvious semantics worth
documenting the same way: `FAIL_OPEN_ELIGIBLE_SEGMENTS` is a *transition*
counter (it fires once when a segment enters the unavailable-with-candidates
state, not per refresh), while `FAIL_OPEN_ELIGIBLE_REQUESTS` fires once *per
query* that touches such a segment. Spelling that out will save operators from
misreading the rates.
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -216,6 +216,14 @@ public class BrokerMeter implements AbstractMetrics.Meter {
public static final BrokerMeter ROUTING_TABLE_UPDATE_FAILURE =
create("ROUTING_TABLE_UPDATE_FAILURE", "failures", false);
+ // Panic routing metrics
Review Comment:
Minor/nit: the phrase "Panic routing" doesn't appear anywhere else in the
codebase — the rest of the code calls this "fail-open". Using the same term
consistently here would make it easier to grep and reason about.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/SegmentStates.java:
##########
@@ -38,12 +38,19 @@ 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;
public SegmentStates(Map<String, List<SegmentInstanceCandidate>>
instanceCandidatesMap, Set<String> servingInstances,
Set<String> unavailableSegments) {
+ this(instanceCandidatesMap, servingInstances, unavailableSegments,
unavailableSegments);
Review Comment:
Minor: this public constructor silently defaults `segmentsWithoutCandidates`
to the full `unavailableSegments` set, which means any `SegmentStates` built
this way treats every unavailable segment as having no candidates (so it never
counts as fail-open eligible). That's a safe/conservative default, but it's
surprising — a short comment explaining the intent would help future callers,
since production now always uses the 4-arg form.
--
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]
