Add broker-side percent of replica metrics - #19221
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19221 +/- ##
============================================
+ Coverage 65.70% 66.95% +1.25%
Complexity 1423 1423
============================================
Files 3439 3453 +14
Lines 218064 218646 +582
Branches 34679 34742 +63
============================================
+ Hits 143289 146405 +3116
+ Misses 63226 60552 -2674
- Partials 11549 11689 +140
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jackie-Jiang
added
the
release-notes
There was a problem hiding this comment.
Pull request overview
Adds broker-side per-table replica-health gauges derived from routing state.
Changes:
- Adds replica percentage, redundancy, and unavailable-segment gauges.
- Integrates metric emission and cleanup with instance-selector lifecycle.
- Adds tests for routing states, selectors, and metric cleanup.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
BrokerGauge.java |
Defines replica-health gauges. |
BaseInstanceSelector.java |
Computes and emits replica health. |
SegmentReplicaHealth.java |
Models replica-health snapshots. |
ReplicaGroupInstanceSelector.java |
Tracks expected replicas for strict routing. |
InstanceSelector.java |
Adds metric cleanup lifecycle API. |
InstanceSelectorConfig.java |
Configures metric emission. |
InstanceSelectorFactory.java |
Disables metrics for sampled views. |
BaseBrokerRoutingManager.java |
Handles metric lifecycle with routing. |
BrokerRoutingManagerTest.java |
Tests routing metric cleanup. |
InstanceSelectorTest.java |
Tests replica-health calculations and gauges. |
Suppressed comments (3)
pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java:2275
- This scenario does not validate the claimed group-wide knockout: both selectors report 66 because
segment0itself has only two of three replicas, so an implementation that never excludes the group fromsegment1still passes. Make different segments lose different replica groups so strict routing reports 33 while balanced routing remains at 66.
SegmentReplicaHealth strictReplicaHealth = strictSelector.getReplicaHealth();
assertEquals(strictReplicaHealth.getMinPercentOfReplicas(), 66);
pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java:2359
- This upsert test also passes through the ordinary per-segment calculation: one segment missing one replica naturally produces 66 without any strict group-wide exclusion. Use two segments missing different replica groups and assert the union knocks both down to one serving replica, so this test actually guards the upsert-specific path.
SegmentReplicaHealth replicaHealth = selector.getReplicaHealth();
assertEquals(replicaHealth.getMinPercentOfReplicas(), 66);
pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java:2438
- This verification is vacuous because the production path emits all three gauges with
setValueOfTableGauge, notsetOrUpdateTableGauge. The preceding check only coversPERCENT_OF_REPLICAS, so accidental emission of the other two gauges by a partial selector would still pass. Verify the actual setter for each gauge.
verify(_brokerMetrics, never()).setOrUpdateTableGauge(eq(TABLE_NAME),
eq(BrokerGauge.SEGMENTS_WITHOUT_REDUNDANCY), any(Supplier.class));
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (SegmentReplicaHealth.shouldMeasure(expectedReplicas)) { | ||
| minPercentOfReplicas = Math.min(minPercentOfReplicas, | ||
| SegmentReplicaHealth.toPercent(servingReplicas, expectedReplicas)); |
There was a problem hiding this comment.
Why is the percentage metric gated on whether the segment is expected to have more than 1 replica? Even for single replica segments, it would be valuable to know whether it's 0% or 100%, no?
There was a problem hiding this comment.
The trade-off to include RF=1 segments is for tables mixed with different RF, e.g. RF=3 for consuming and completed segments and RF=1 for tier. The gauge value would be dominated by those segment reporting 0% while rolling restart or rebalance. We're not able to measure the replica percent that we're interested in.
imho RF=1 segments are not replicated at all thus not relevant to the concept "replica". We are able to track those by unavailable segment gauge though. wdyt?
| /// Number of the table's segments that are down to their last routable replica, or have none left. | ||
| /// Segments assigned a single replica are excluded, since they never had redundancy to lose. Recently created | ||
| /// segments are excluded on the same terms as [#PERCENT_OF_REPLICAS]. | ||
| SEGMENTS_WITHOUT_REDUNDANCY("segments", false); |
There was a problem hiding this comment.
What is the purpose of this gauge? I feel PERCENT_OF_REPLICAS should be good enough
There was a problem hiding this comment.
With only PERCENT_OF_REPLICAS one is not able to tell apart one segment is affected or all segments are affected. These two together are better to assess the healthy status of the table
There was a problem hiding this comment.
If you want to know how many segments contribute to the lowest percentage, we should define it as the segments of the lowest PERCENT_OF_REPLICAS, instead of the ones with single replica
There was a problem hiding this comment.
With only PERCENT_OF_REPLICAS one is not able to tell apart one segment is affected or all segments are affected. These two together are better to assess the healthy status of the table
But if there's some segments that are at 2/3 replicas online and all the rest are at 3/3, this metric would still be 0? +1 to Jackie's point above.
There was a problem hiding this comment.
We are also interested in how many segments are down to single replica (they're at risk of becoming unavailable), in the case 1 segment down to 0 and 100 segments down to 1, showing only how many is down to lowest would only reflects the one segment, instead of showing 100 at risk.
Current implementation shows the lowest 0%, 1 unavailable, and 101 without redundancy.
There was a problem hiding this comment.
IMO, once we got unavailable segment, it doesn't really matter how many segments are at risk because the table is already down. Knowing the amount of segments causing lower replication could be useful to understand if it is a single segment issue or multiple segments issue.
One concrete example would be, people might want at least 2 replicas up, and we should report how many segments are at replication 2, instead of 1. I feel it is more useful to track the segment count with lowest replications.
yashmayya
left a comment
yashmayya
left a comment
There was a problem hiding this comment.
@Jackie-Jiang @J-HowHuang I'm wondering if the metric emission logic should maybe sit in the routing manager instead of instance selector code? We'd not need all the _emitReplicaHealthMetrics checks because it already knows main vs sampler, it already tracks disabled tables, removeRoutingInternal already has the table name, so it can call removeTableGauge directly and InstanceSelector.removeMetrics() doesn't need to exist, etc.
Description
Currently it's
SegmentStatusCheckerin controllers reporting replica-related gauges. There are few down sides:For example, it doesn't account for strict replica group limitation to exclude unavailable instance for other segments in the entire replica groups
A better idea is to piggy-back on what we already have in broker's instance selector of a table's routing entry, i.e. grace period for new segments, EV & IS change listener, strict replica group handling.
Changes
Three gauges, all per-table, all instantaneous, emitted from the broker's routing table:
BrokerGauge.PERCENT_OF_REPLICASWorst segment's routable replicas as a percentage of its assigned replicas — the minimum across the table, each segment measured against its own assignment.
Excluding segments with only one replica, or new segment that's fresher than
pinot.broker.new.segment.expiration.seconds.BrokerGauge.SEGMENTS_WITHOUT_REDUNDANCYHow many segments are down to their last routable replica, or have none left — the blast radius behind the percentage.
Excluding segments with only one replica, or new segment that's fresher than
pinot.broker.new.segment.expiration.seconds.BrokerGauge.UNAVAILABLE_SEGMENTSHow many segments cannot be routed anywhere, i.e. what queries actually fail on with
BROKER_SEGMENT_UNAVAILABLE.Segments with only one replica is included.
Excluding new segment that's fresher than
pinot.broker.new.segment.expiration.seconds.Note
Since now this is a broker metric, each table would get as many time series as it's assigned brokers. Monitor the
minover the time series if interested in the worst scenario.