9aman commented on code in PR #19094:
URL: https://github.com/apache/pinot/pull/19094#discussion_r3682458504
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java:
##########
@@ -417,17 +417,29 @@ private boolean updateSegmentMetrics(String
tableNameWithType, TableConfig table
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 are just created/pushed/committed
to avoid false alerts because it is
+ // expected for servers to take some time to load them. We derive
the "newly created" timestamp per status:
+ // - consuming (IN_PROGRESS) and pauseless committing
(COMMITTING) segments: use creation time. A
+ // COMMITTING segment has finished consuming but its immutable
segment is still being built/loaded on
+ // the replicas, so it is expected to be transiently
under-replicated during that window.
+ // - pushed/committed segments: use push time when it is set.
Real-time (LLC) committed (DONE) segments
+ // never populate push time, so we fall back to creation time
(which is populated at creation and
+ // persists) instead of leaving it at Long.MIN_VALUE, so a
just-committed real-time segment still gets
+ // the grace window while its replicas finish loading.
+ // 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();
+ Status segmentStatus = segmentZKMetadata.getStatus();
+ long creationTimeMs;
Review Comment:
This approach worked for `IN_PROGRESS` segments because creation time is
equal to `mtime` for these segment.
For COMMITTING and DONE, creation time is a wrong proxy e.g.
1. Segment created at 1:00 pm IST
2. Segment Ingested and marked committing at 2:00 pm IST
3. Segment takes `2 minutes` for the state transition from CONSUMING to
ONLINE
As per our expectation, the segment should not be considered for the check.
The condition below will still flag that segment as valid for status check and
will fail the check.
If we use mtime, the below condition
`if (mTimeMs > evSnapshotTimestamp - _waitForPushTimeSeconds * 1000L) `
will be false and the segment will not be picked for the check.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java:
##########
@@ -417,17 +417,29 @@ private boolean updateSegmentMetrics(String
tableNameWithType, TableConfig table
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 are just created/pushed/committed
to avoid false alerts because it is
+ // expected for servers to take some time to load them. We derive
the "newly created" timestamp per status:
+ // - consuming (IN_PROGRESS) and pauseless committing
(COMMITTING) segments: use creation time. A
+ // COMMITTING segment has finished consuming but its immutable
segment is still being built/loaded on
+ // the replicas, so it is expected to be transiently
under-replicated during that window.
+ // - pushed/committed segments: use push time when it is set.
Real-time (LLC) committed (DONE) segments
+ // never populate push time, so we fall back to creation time
(which is populated at creation and
+ // persists) instead of leaving it at Long.MIN_VALUE, so a
just-committed real-time segment still gets
+ // the grace window while its replicas finish loading.
+ // 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();
+ Status segmentStatus = segmentZKMetadata.getStatus();
+ long creationTimeMs;
Review Comment:
```
public static ZNRecord getZnRecord(ZkHelixPropertyStore<ZNRecord>
propertyStore, String path) {
Stat stat = new Stat();
ZNRecord znRecord = propertyStore.get(path, stat, AccessOption.PERSISTENT);
if (znRecord != null) {
znRecord.setCreationTime(stat.getCtime());
znRecord.setModifiedTime(stat.getMtime()); // <-- here
znRecord.setVersion(stat.getVersion());
}
return znRecord;
}
```
We already get Mtime here. Maybe we can add `getModifiedTime()` on
SegmentZKMetadata;
--
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]
