J-HowHuang commented on code in PR #19252:
URL: https://github.com/apache/pinot/pull/19252#discussion_r3799236893
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/SegmentStatusCheckerTest.java:
##########
@@ -626,6 +671,198 @@ public void
realtimeCommittingSegmentWithinGraceNotUnderReplicated() {
ControllerGauge.SEGMENTS_WITH_LESS_REPLICAS), 0);
}
+ /// When a segment's znode stat is unavailable, the grace window falls back
to the metadata's creation time. A freshly
+ /// created CONSUMING segment (mtime == creation time) must therefore still
be graced, so a lost stat cannot turn into
+ /// a false under-replication alert.
+ @Test
+ public void
realtimeConsumingSegmentWithoutZNodeStatFallsBackToCreationTime() {
+ TableConfig tableConfig =
+ new
TableConfigBuilder(TableType.REALTIME).setTableName(RAW_TABLE_NAME).setTimeColumnName("timeColumn")
+ .setNumReplicas(3).setStreamConfigs(getStreamConfigMap()).build();
+
+ String seg = new LLCSegmentName(RAW_TABLE_NAME, 1, 5,
System.currentTimeMillis()).getSegmentName();
+ IdealState idealState = new IdealState(REALTIME_TABLE_NAME);
+ idealState.setPartitionState(seg, "pinot1", "CONSUMING");
+ idealState.setPartitionState(seg, "pinot2", "CONSUMING");
+ idealState.setPartitionState(seg, "pinot3", "CONSUMING");
+ idealState.setReplicas("3");
+ idealState.setRebalanceMode(IdealState.RebalanceMode.CUSTOMIZED);
+
+ // Just created: only 1 of 3 replicas has started consuming.
+ ExternalView externalView = new ExternalView(REALTIME_TABLE_NAME);
+ externalView.setState(seg, "pinot1", "CONSUMING");
+ externalView.setState(seg, "pinot2", "OFFLINE");
+ externalView.setState(seg, "pinot3", "OFFLINE");
+
+ PinotHelixResourceManager resourceManager =
mock(PinotHelixResourceManager.class);
+
when(resourceManager.getHelixInstanceConfig(any())).thenReturn(newQuerableInstanceConfig("any"));
+
when(resourceManager.getTableConfig(REALTIME_TABLE_NAME)).thenReturn(tableConfig);
+
when(resourceManager.getAllTables()).thenReturn(List.of(REALTIME_TABLE_NAME));
+
when(resourceManager.getTableIdealState(REALTIME_TABLE_NAME)).thenReturn(idealState);
+
when(resourceManager.getTableExternalView(REALTIME_TABLE_NAME)).thenReturn(externalView);
+ // Creation time is now, and no znode stat is available -> the fallback
must keep the segment within the grace
+ // window
+ SegmentZKMetadata consumingSegmentZKMetadata =
mockConsumingSegmentZKMetadata(System.currentTimeMillis());
+ mockSegmentsZKMetadata(resourceManager, REALTIME_TABLE_NAME, Map.of(seg,
consumingSegmentZKMetadata));
+
+ ZkHelixPropertyStore<ZNRecord> propertyStore =
mock(ZkHelixPropertyStore.class);
+ when(resourceManager.getPropertyStore()).thenReturn(propertyStore);
+ ZNRecord znRecord = new ZNRecord("0");
+ znRecord.setSimpleField(CommonConstants.Segment.Realtime.END_OFFSET,
"10000");
+ when(propertyStore.get(anyString(), any(), anyInt())).thenReturn(znRecord);
+
+ // 1h grace window; the segment was just created, so it must be skipped
and the table stays fully replicated.
+ runSegmentStatusChecker(resourceManager, 3600);
+ assertEquals(MetricValueUtils.getTableGaugeValue(_controllerMetrics,
REALTIME_TABLE_NAME,
+ ControllerGauge.PERCENT_OF_REPLICAS), 100);
+ assertEquals(MetricValueUtils.getTableGaugeValue(_controllerMetrics,
REALTIME_TABLE_NAME,
+ ControllerGauge.SEGMENTS_WITH_LESS_REPLICAS), 0);
+ }
+
+ /// The whole table's metadata and znode stats come back from batched reads,
so each segment must be matched to its
+ /// own metadata (by name) and its own stat (by position) rather than to
whichever entry happens to sit at its index.
+ /// Every segment is under-replicated and carries a distinct size, exactly
one segment was pushed recently enough to
+ /// be graced, and the last segment is the only one with 4 replicas so it
alone determines PERCENT_OF_REPLICAS.
+ /// Together the gauges pin that every segment was examined under its own
name, that the grace window applied to
+ /// exactly the segment whose znode stat is recent, and that the sizes
accumulate. One segment has no ZK metadata,
+ /// which shifts the alignment if the pairing gets it wrong.
+ @Test
+ public void segmentsStayAlignedWithTheirBatchedMetadata() {
+ TableConfig tableConfig =
+ new
TableConfigBuilder(TableType.OFFLINE).setTableName(RAW_TABLE_NAME).setNumReplicas(2).build();
+
+ int numSegments = 7;
+ // Distinct positions, none of them the last segment, which carries its
own marker below
+ int segmentWithoutZKMetadata = 3;
+ int recentlyPushedSegment = 1;
+ // The last segment is the marker that pins name-to-metadata pairing: it
is the only one whose replica ratio is 1/4
+ // rather than 1/2, so PERCENT_OF_REPLICAS drops to 25 only if this exact
segment was examined.
+ int lowReplicaSegment = numSegments - 1;
+ long oldPushTimeMs = 11111L;
+
+ IdealState idealState = new IdealState(OFFLINE_TABLE_NAME);
+ ExternalView externalView = new ExternalView(OFFLINE_TABLE_NAME);
+ Map<String, SegmentZKMetadata> segmentZKMetadataMap = new HashMap<>();
+ Map<String, Long> segmentZNodeMTimesMs = new HashMap<>();
+ long expectedTableCompressedSize = 0;
+ for (int i = 0; i < numSegments; i++) {
+ String segment = "myTable_" + i;
+ int numReplicas = i == lowReplicaSegment ? 4 : 2;
+ // Every segment is under-replicated, so any segment that is examined
and not graced must be counted
+ for (int replica = 1; replica <= numReplicas; replica++) {
+ idealState.setPartitionState(segment, "pinot" + replica, "ONLINE");
+ externalView.setState(segment, "pinot" + replica, replica == 1 ?
"ONLINE" : "OFFLINE");
+ }
+ if (i == segmentWithoutZKMetadata) {
+ continue;
+ }
+ // Distinct size per segment so that the total pins which metadata was
attributed to which segment
+ long sizeInBytes = 1000L + i;
+ segmentZKMetadataMap.put(segment,
mockPushedSegmentZKMetadata(sizeInBytes, oldPushTimeMs));
+ segmentZNodeMTimesMs.put(segment, i == recentlyPushedSegment ?
System.currentTimeMillis() : oldPushTimeMs);
+ expectedTableCompressedSize += sizeInBytes;
+ }
+ idealState.setReplicas("2");
+ idealState.setRebalanceMode(IdealState.RebalanceMode.CUSTOMIZED);
+
+ PinotHelixResourceManager resourceManager =
mock(PinotHelixResourceManager.class);
+
when(resourceManager.getHelixInstanceConfig(any())).thenReturn(newQuerableInstanceConfig("any"));
+
when(resourceManager.getAllTables()).thenReturn(List.of(OFFLINE_TABLE_NAME));
+
when(resourceManager.getTableConfig(OFFLINE_TABLE_NAME)).thenReturn(tableConfig);
+
when(resourceManager.getTableIdealState(OFFLINE_TABLE_NAME)).thenReturn(idealState);
+
when(resourceManager.getTableExternalView(OFFLINE_TABLE_NAME)).thenReturn(externalView);
+ mockSegmentsZKMetadata(resourceManager, OFFLINE_TABLE_NAME,
segmentZKMetadataMap, segmentZNodeMTimesMs);
+
+ ZkHelixPropertyStore<ZNRecord> propertyStore =
mock(ZkHelixPropertyStore.class);
+ when(resourceManager.getPropertyStore()).thenReturn(propertyStore);
+
+ // 10min grace window, so only the recently pushed segment is skipped
+ runSegmentStatusChecker(resourceManager, 600, mock(TableSizeReader.class));
+
+ assertEquals(MetricValueUtils.getTableGaugeValue(_controllerMetrics,
OFFLINE_TABLE_NAME,
+ ControllerGauge.SEGMENT_COUNT), numSegments);
+ // Every segment except the one without ZK metadata and the graced one
must be counted. A segment paired with the
+ // wrong metadata or the wrong znode stat drops out of, or into, this
count.
+ assertEquals(MetricValueUtils.getTableGaugeValue(_controllerMetrics,
OFFLINE_TABLE_NAME,
+ ControllerGauge.SEGMENTS_WITH_LESS_REPLICAS), numSegments - 2);
+ // Only the last segment has a 1-of-4 ratio, so this is 25 only if that
segment was examined under its own name
+ assertEquals(MetricValueUtils.getTableGaugeValue(_controllerMetrics,
OFFLINE_TABLE_NAME,
+ ControllerGauge.PERCENT_OF_REPLICAS), 25);
+ // The sizes accumulate over exactly the segments that have metadata
(including the graced one, whose size is
+ // counted before the grace check)
+ assertEquals(MetricValueUtils.getTableGaugeValue(_controllerMetrics,
OFFLINE_TABLE_NAME,
+ ControllerGauge.TABLE_COMPRESSED_SIZE), expectedTableCompressedSize);
+
+ // A single batched read must cover the whole table, metadata and znode
stats together: more than one call means the
+ // per-segment reads crept back in some form
+ ArgumentCaptor<List<String>> segmentNamesCaptor =
ArgumentCaptor.forClass(List.class);
+
verify(resourceManager).getSegmentsZKMetadataForSegmentNames(eq(OFFLINE_TABLE_NAME),
segmentNamesCaptor.capture(),
+ any());
+ verify(propertyStore, never()).getStats(any(), anyInt());
+ List<String> requestedSegments = segmentNamesCaptor.getValue();
+ assertEquals(requestedSegments.size(), numSegments);
+ assertEquals(new HashSet<>(requestedSegments),
idealState.getPartitionSet());
+ }
+
+ /// When not a single segment's ZK metadata can be read the table's gauges
must be left alone rather than reset to
+ /// all-green values, because an all-green gauge silences the alerts that a
stale one would still fire. Regression
+ /// test for a whole-table ZK read failure being reported as a perfectly
healthy table.
+ @Test
+ public void tableWithoutAnyReadableSegmentZKMetadataKeepsItsGauges() {
Review Comment:
Good catch, I verified the entire test suite passes together but
`noSegmentZKMetadataTest` alone fails. There are interdependency between test
cases. Let me update
--
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]
