swaminathanmanish commented on code in PR #19094:
URL: https://github.com/apache/pinot/pull/19094#discussion_r3684034743
##########
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:
Yes makes sense. Using mtime for both in_progress & committing, if its
there, otherwise falling back to creation time
--
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]
