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.
| BaseInstanceSelector strictSelector = | ||
| createReplicaHealthSelector(STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE, REPLICA_INSTANCES, | ||
| idealStateAssignment, externalViewAssignment); | ||
| SegmentReplicaHealth strictReplicaHealth = strictSelector.getReplicaHealth(); | ||
| assertEquals(strictReplicaHealth.getMinPercentOfReplicas(), 66); | ||
|
|
||
| // Without the strict guarantee only the segment that is actually missing a replica is affected | ||
| createOldSegments(List.of("segment0")); | ||
| BaseInstanceSelector balancedSelector = | ||
| createReplicaHealthSelector(BALANCED_INSTANCE_SELECTOR, REPLICA_INSTANCES, idealStateAssignment, | ||
| externalViewAssignment); | ||
| SegmentReplicaHealth balancedReplicaHealth = balancedSelector.getReplicaHealth(); | ||
| assertEquals(balancedReplicaHealth.getMinPercentOfReplicas(), 66); |
There was a problem hiding this comment.
If both are 66% , what's the point of this test?
There was a problem hiding this comment.
hmm good point, updated the test to cover down on different servers
| 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
| /// | ||
| /// Kept sparse deliberately: an absent entry means "as many candidates as the ideal state assigns", | ||
| /// so a healthy table stores nothing here. Read it through [#getExpectedReplicas]. | ||
| protected final Map<String, Integer> _oldSegmentExpectedReplicasMap = new HashMap<>(); |
There was a problem hiding this comment.
Do we need to track this map? Can we update the min percentage as we add segment to the routing?
There was a problem hiding this comment.
Because the actual percentage would be answered at refreshSegmentStates. Not only EV-IS change triggers it, instance state change (e.g. k8s pod not ready) also refresh the availability thus percentage. We need to keep track of this.
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.