This is an automated email from the ASF dual-hosted git repository.
9aman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new c6da079b9bd Grace pauseless COMMITTING/committed real-time segments in
SegmentStatusChecker (#19094)
c6da079b9bd is described below
commit c6da079b9bd7b92b9f8087e60edd53dbf722641d
Author: swaminathanmanish <[email protected]>
AuthorDate: Mon Aug 3 10:18:26 2026 +0530
Grace pauseless COMMITTING/committed real-time segments in
SegmentStatusChecker (#19094)
* Grace pauseless COMMITTING and committed real-time segments in
SegmentStatusChecker
SegmentStatusChecker skips just-created/pushed segments from the
replica-availability
check for a grace window (waitForPushTime). The grace timestamp was derived
as:
IN_PROGRESS -> creation time, everything else -> push time. Real-time (LLC)
committed
segments never populate push time (it stays Long.MIN_VALUE), so a
just-committed
segment was never graced. Pauseless ingestion makes this worse: a segment
enters
COMMITTING (done consuming, immutable segment still being built/loaded on
the
replicas) and is transiently under-replicated for the build window, yet
COMMITTING
was treated as non-IN_PROGRESS and checked immediately.
This produces false percentOfReplicas dips
(SegmentReplicasCriticallyLowForHATable)
for high-ingest pauseless tables even though the table is fully redundant.
Fix: grace the transient window by keying on a populated timestamp:
- IN_PROGRESS and COMMITTING -> creation time
- committed/pushed -> push time when set, else fall back to creation time
The existing waitForPushTime window is reused; once a segment is older than
it and
still under-replicated it is checked normally, so genuinely stuck commits
and real
replica losses still alert.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
* Use segment znode mtime for the status-checker grace window
Address review feedback: creation time is the wrong grace clock for
COMMITTING/DONE real-time segments (it marks when consumption started,
which can be long before commit). Use the segment ZK znode modification
time (mtime), which tracks the last state change — creation for
IN_PROGRESS, the commit transition for COMMITTING/DONE, and push time for
offline segments. Read it via propertyStore.getStat(...), falling back to
creation time if the stat is unavailable. Update the COMMITTING regression
tests to drive the clock via the znode mtime.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
* Drive grace clock via znode mtime in status-checker push/uploaded tests
missingEVPartitionPushTest and missingEVUploadedConsumingTest relied on
the grace window skipping just-pushed/updated segments. Now that the grace
keys off the segment znode mtime, mock propertyStore.getStat(...) to supply
the mtime (recent for graced segments, old otherwise) instead of leaning on
the getCreationTime() fallback, so the tests exercise the real code path.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
* Use /// markdown doc comments for the two COMMITTING tests
master added a checkstyle rule banning /** */ Javadoc in favor of ///
(JEP 467) markdown doc comments. Convert the two doc comments this PR
introduced so the linter passes on the merged-with-master build.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
---
.../controller/helix/SegmentStatusChecker.java | 23 ++--
.../controller/helix/SegmentStatusCheckerTest.java | 121 +++++++++++++++++++++
2 files changed, 136 insertions(+), 8 deletions(-)
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java
index e0c28492f5a..4ed16098a86 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java
@@ -63,6 +63,7 @@ import
org.apache.pinot.spi.utils.CommonConstants.Segment.Realtime.Status;
import org.apache.pinot.spi.utils.IngestionConfigUtils;
import org.apache.pinot.spi.utils.TimeUtils;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.apache.zookeeper.data.Stat;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -407,18 +408,24 @@ public class SegmentStatusChecker extends
ControllerPeriodicTask<SegmentStatusCh
tableCompressedSize += sizeInBytes;
}
- // NOTE: We want to skip segments that are just created/pushed to avoid
false alerts because it is expected for
- // servers to take some time to load them. For consuming
(IN_PROGRESS) segments, we use creation time from
- // the ZK metadata; for pushed segments, we use push time from the
ZK metadata. Both of them are the time
- // when segment is newly created. For committed segments from
real-time table, push time doesn't exist, and
- // creationTimeMs will be Long.MIN_VALUE, which is fine because we
want to include them in the check.
+ // NOTE: We want to skip segments that were recently
created/committed/pushed to avoid false alerts, because it
+ // is expected for servers to take some time to load them. We use
the segment ZK znode's modification time
+ // (mtime), which tracks the last state change: creation for
consuming (IN_PROGRESS) segments, the
+ // CONSUMING -> COMMITTING -> ONLINE commit transition for
real-time (LLC) segments, and the push time for
+ // offline segments. Creation time is not a correct proxy for a
COMMITTING/DONE segment: it marks when
+ // consumption STARTED, which can be long before commit, so a
segment still transitioning to ONLINE would
+ // be flagged. If the znode stat is unavailable we fall back to
creation time (mtime == creation for a
+ // freshly created IN_PROGRESS segment).
+ // The grace window is _waitForPushTimeSeconds. Once a segment is
older than it and still
+ // under-replicated, it is checked normally, so genuinely stuck
commits and real replica losses still alert.
// The comparison uses evSnapshotTimestamp instead of
System.currentTimeMillis() because for large tables
// with many segments, the status check can take several minutes.
A segment updated after
// the EV snapshot was taken but before this individual segment
check runs could be incorrectly flagged as
// OFFLINE when using current time.
- long creationTimeMs = segmentZKMetadata.getStatus() ==
Status.IN_PROGRESS ? segmentZKMetadata.getCreationTime()
- : segmentZKMetadata.getPushTime();
- if (creationTimeMs > evSnapshotTimestamp - _waitForPushTimeSeconds *
1000L) {
+ Stat segmentStat = propertyStore == null ? null : propertyStore.getStat(
+
ZKMetadataProvider.constructPropertyStorePathForSegment(tableNameWithType,
segment), AccessOption.PERSISTENT);
+ long refTimeMs = segmentStat != null ? segmentStat.getMtime() :
segmentZKMetadata.getCreationTime();
+ if (refTimeMs > evSnapshotTimestamp - _waitForPushTimeSeconds * 1000L) {
continue;
}
diff --git
a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/SegmentStatusCheckerTest.java
b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/SegmentStatusCheckerTest.java
index 8df09a39a23..b5d390d0215 100644
---
a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/SegmentStatusCheckerTest.java
+++
b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/SegmentStatusCheckerTest.java
@@ -57,6 +57,7 @@ import org.apache.pinot.spi.utils.JsonUtils;
import org.apache.pinot.spi.utils.TimeUtils;
import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.apache.zookeeper.data.Stat;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
@@ -560,6 +561,118 @@ public class SegmentStatusCheckerTest {
return segmentZKMetadata;
}
+ // A pauseless COMMITTING segment: done consuming but its immutable segment
is still being built/loaded on the
+ // replicas. The grace check keys off the segment znode's modification time
(mtime), which the test drives via
+ // propertyStore.getStat(...); the metadata here only supplies status and
size.
+ private SegmentZKMetadata mockCommittingSegmentZKMetadata() {
+ SegmentZKMetadata segmentZKMetadata = mock(SegmentZKMetadata.class);
+ when(segmentZKMetadata.getStatus()).thenReturn(Status.COMMITTING);
+ when(segmentZKMetadata.getSizeInBytes()).thenReturn(-1L);
+ return segmentZKMetadata;
+ }
+
+ // A ZK Stat whose modification time (mtime) is set to the given epoch
millis, used to drive the grace-window check.
+ private Stat mockStatWithMTime(long mTimeMs) {
+ Stat stat = new Stat();
+ stat.setMtime(mTimeMs);
+ return stat;
+ }
+
+ /// A pauseless COMMITTING segment whose replicas are still building (only
1/3 ONLINE in the external view) must not
+ /// be counted as under-replicated while it is within the grace window, so
percentOfReplicas stays at 100. Regression
+ /// test for the SegmentReplicasCriticallyLowForHATable false positive on
pauseless tables.
+ @Test
+ public void realtimeCommittingSegmentWithinGraceNotUnderReplicated() {
+ 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", "ONLINE");
+ idealState.setPartitionState(seg, "pinot2", "ONLINE");
+ idealState.setPartitionState(seg, "pinot3", "ONLINE");
+ idealState.setReplicas("3");
+ idealState.setRebalanceMode(IdealState.RebalanceMode.CUSTOMIZED);
+
+ // Just committed: only 1 of 3 replicas ONLINE, the other two still
building the immutable segment.
+ ExternalView externalView = new ExternalView(REALTIME_TABLE_NAME);
+ externalView.setState(seg, "pinot1", "ONLINE");
+ 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);
+ SegmentZKMetadata committingSegmentZKMetadata =
mockCommittingSegmentZKMetadata();
+ when(resourceManager.getSegmentZKMetadata(REALTIME_TABLE_NAME,
seg)).thenReturn(committingSegmentZKMetadata);
+
+ 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);
+ // Just committed: znode mtime is now, within the grace window.
+ when(propertyStore.getStat(anyString(),
anyInt())).thenReturn(mockStatWithMTime(System.currentTimeMillis()));
+
+ // 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);
+ }
+
+ /// A COMMITTING segment that has been under-replicated for longer than the
grace window is a genuinely stuck commit
+ /// and must still be flagged (percentOfReplicas drops), so the grace does
not mask real problems.
+ @Test
+ public void realtimeCommittingSegmentBeyondGraceUnderReplicated() {
+ 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", "ONLINE");
+ idealState.setPartitionState(seg, "pinot2", "ONLINE");
+ idealState.setPartitionState(seg, "pinot3", "ONLINE");
+ idealState.setReplicas("3");
+ idealState.setRebalanceMode(IdealState.RebalanceMode.CUSTOMIZED);
+
+ ExternalView externalView = new ExternalView(REALTIME_TABLE_NAME);
+ externalView.setState(seg, "pinot1", "ONLINE");
+ 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);
+ SegmentZKMetadata committingSegmentZKMetadata =
mockCommittingSegmentZKMetadata();
+ when(resourceManager.getSegmentZKMetadata(REALTIME_TABLE_NAME,
seg)).thenReturn(committingSegmentZKMetadata);
+
+ 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);
+ // Committed 2h ago (znode mtime), still under-replicated -> a stuck
commit, must not be graced.
+ when(propertyStore.getStat(anyString(), anyInt()))
+ .thenReturn(mockStatWithMTime(System.currentTimeMillis() - 7200000L));
+
+ // 1h grace window; the segment is 2h old and still 1/3 replicas up, so it
must be flagged (33%).
+ runSegmentStatusChecker(resourceManager, 3600);
+ assertEquals(MetricValueUtils.getTableGaugeValue(_controllerMetrics,
REALTIME_TABLE_NAME,
+ ControllerGauge.PERCENT_OF_REPLICAS), 33);
+ assertEquals(MetricValueUtils.getTableGaugeValue(_controllerMetrics,
REALTIME_TABLE_NAME,
+ ControllerGauge.SEGMENTS_WITH_LESS_REPLICAS), 1);
+ }
+
@Test
public void missingEVPartitionTest() {
IdealState idealState = new IdealState(OFFLINE_TABLE_NAME);
@@ -684,6 +797,12 @@ public class SegmentStatusCheckerTest {
ZkHelixPropertyStore<ZNRecord> propertyStore =
mock(ZkHelixPropertyStore.class);
when(resourceManager.getPropertyStore()).thenReturn(propertyStore);
+ // myTable_2 was just pushed (znode mtime is now) so it is within the
grace window and skipped; the others were
+ // pushed long ago.
+ when(propertyStore.getStat(anyString(), anyInt())).thenAnswer(inv -> {
+ String path = inv.getArgument(0);
+ return mockStatWithMTime(path.endsWith("myTable_2") ?
System.currentTimeMillis() : 11111L);
+ });
runSegmentStatusChecker(resourceManager, 600);
verifyControllerMetrics(OFFLINE_TABLE_NAME, 0, 3, 3, 2, 100, 0, 100, 0,
3702);
@@ -707,6 +826,8 @@ public class SegmentStatusCheckerTest {
ZkHelixPropertyStore<ZNRecord> propertyStore =
mock(ZkHelixPropertyStore.class);
when(resourceManager.getPropertyStore()).thenReturn(propertyStore);
+ // Both segments were just updated/created (znode mtime is now), so they
are within the grace window and skipped.
+ when(propertyStore.getStat(anyString(),
anyInt())).thenReturn(mockStatWithMTime(System.currentTimeMillis()));
runSegmentStatusChecker(resourceManager, 600);
verifyControllerMetrics(REALTIME_TABLE_NAME, 0, 2, 2, 1, 100, 0, 100, 0,
1234);
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
