J-HowHuang commented on code in PR #19221:
URL: https://github.com/apache/pinot/pull/19221#discussion_r3797893285
##########
pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java:
##########
@@ -1927,4 +1932,466 @@ public void testReplicaGroupAdaptiveServerSelector() {
assertEquals(selectedResult.getLeft(), expectedSelection);
}
+
+ // Replica health metrics
+ //
+ // The scenarios below all use the same three instances and assert on the
TableReplicaHealth the
+ // selector derives, since that is what the gauges are emitted from. Each
segment's percentage is
+ // measured against the replicas its own ideal state assigns, so segments do
not have to be uniformly
+ // replicated for the numbers to make sense.
+
+ private static final String REPLICA_INSTANCE_0 = "instance0";
+ private static final String REPLICA_INSTANCE_1 = "instance1";
+ private static final String REPLICA_INSTANCE_2 = "instance2";
+ private static final Set<String> REPLICA_INSTANCES =
+ ImmutableSet.of(REPLICA_INSTANCE_0, REPLICA_INSTANCE_1,
REPLICA_INSTANCE_2);
+
+ /// Returns the ideal state assignment placing the segment on all three
instances as ONLINE.
+ private static List<Pair<String, String>> allOnline() {
+ return List.of(new ImmutablePair<>(REPLICA_INSTANCE_0, ONLINE), new
ImmutablePair<>(REPLICA_INSTANCE_1, ONLINE),
+ new ImmutablePair<>(REPLICA_INSTANCE_2, ONLINE));
+ }
+
+ /// Returns an assignment placing the segment on two of the three instances
as ONLINE.
+ private static List<Pair<String, String>> twoReplicas() {
+ return List.of(new ImmutablePair<>(REPLICA_INSTANCE_0, ONLINE), new
ImmutablePair<>(REPLICA_INSTANCE_1, ONLINE));
+ }
+
+ /// Returns an external view assignment where the first `numOnline` of the
three instances are ONLINE
+ /// and the rest are OFFLINE, so the segment looks partially loaded.
+ private static List<Pair<String, String>> partiallyOnline(int numOnline) {
+ List<String> instances = List.of(REPLICA_INSTANCE_0, REPLICA_INSTANCE_1,
REPLICA_INSTANCE_2);
+ List<Pair<String, String>> assignment = new ArrayList<>(instances.size());
+ for (int i = 0; i < instances.size(); i++) {
+ assignment.add(new ImmutablePair<>(instances.get(i), i < numOnline ?
ONLINE : OFFLINE));
+ }
+ return assignment;
+ }
+
+ /// Returns an external view assignment where every instance but
`offlineInstance` is ONLINE, so that
+ /// different segments can be made to lose different replicas.
+ private static List<Pair<String, String>> onlineExcept(String
offlineInstance) {
+ List<Pair<String, String>> assignment = new
ArrayList<>(REPLICA_INSTANCES.size());
+ for (String instance : List.of(REPLICA_INSTANCE_0, REPLICA_INSTANCE_1,
REPLICA_INSTANCE_2)) {
+ assignment.add(new ImmutablePair<>(instance,
instance.equals(offlineInstance) ? OFFLINE : ONLINE));
+ }
+ return assignment;
+ }
+
+ private BaseInstanceSelector createReplicaHealthSelector(String
selectorType, Set<String> enabledInstances,
+ Map<String, List<Pair<String, String>>> idealStateAssignment,
+ Map<String, List<Pair<String, String>>> externalViewAssignment) {
+ // Sorted so that the order the selector looks up segment metadata in is
deterministic, which is what
+ // the stub set up by createSegmentCreationTimes matches on
+ return (BaseInstanceSelector) createTestInstanceSelector(selectorType,
enabledInstances,
+ createIdealState(idealStateAssignment),
createExternalView(externalViewAssignment),
+ new TreeSet<>(externalViewAssignment.keySet()));
+ }
+
+ /// Stubs the segment metadata lookup with the given creation times, in the
order the selector reads them.
+ private void createSegmentCreationTimes(Map<String, Long>
creationTimeMsBySegment) {
+ List<Pair<String, Long>> creationTimes = new
ArrayList<>(creationTimeMsBySegment.size());
+ for (String segment : new TreeSet<>(creationTimeMsBySegment.keySet())) {
+ creationTimes.add(new ImmutablePair<>(segment,
creationTimeMsBySegment.get(segment)));
+ }
+ createSegments(creationTimes);
+ }
+
+ /// Marks the given segments as created long enough ago that they are no
longer treated as new, so that
+ /// they count towards the replica health even though their external view
has not converged.
+ private void createOldSegments(List<String> segments) {
+ long creationTimeMs = _mutableClock.millis() -
NEW_SEGMENT_EXPIRATION_MILLIS - 1;
+ Map<String, Long> creationTimes = new HashMap<>();
+ for (String segment : segments) {
+ creationTimes.put(segment, creationTimeMs);
+ }
+ createSegmentCreationTimes(creationTimes);
+ }
+
+ @Test(dataProvider = "selectorType")
+ public void testReplicaHealthFullyReplicated(String selectorType) {
+ // Every segment is ONLINE everywhere the ideal state assigns it
+ BaseInstanceSelector selector = createReplicaHealthSelector(selectorType,
REPLICA_INSTANCES,
+ Map.of("segment0", allOnline(), "segment1", allOnline()),
+ Map.of("segment0", allOnline(), "segment1", allOnline()));
+
+ TableReplicaHealth replicaHealth = selector.getReplicaHealth();
+ assertEquals(replicaHealth.getMinPercentOfReplicas(), 100);
+ assertEquals(replicaHealth.getNumUnavailableSegments(), 0);
+ // Every measured segment sits at the minimum when the minimum is 100, so
the count is the measured
+ // population rather than 0 - it counts what the percentage speaks for,
not what is wrong
+ assertEquals(replicaHealth.getNumSegmentsAtMinPercentOfReplicas(), 2);
Review Comment:
It feels odd to me to explicitly exclude the number when it's 100% healthy.
I think this behavior fits best to its name and keep things simple.
--
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]
