shauryachats commented on code in PR #18855:
URL: https://github.com/apache/pinot/pull/18855#discussion_r3482992424
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java:
##########
@@ -82,7 +84,7 @@ public void init(IdealState idealState, ExternalView
externalView, Set<String> o
listener.init(idealState, externalView, segments, znRecords);
}
for (int i = 0; i < numSegments; i++) {
- if (znRecords.get(i) != null) {
+ if (!isConsumingInExternalView(externalView, segments.get(i))) {
Review Comment:
Good catch! Just realised there is actually a deeper issue here beyond just
null ZNRecords.
Even when the ZNRecord is non-null, there's a brief race window when a
segment commits: the server updates the ExternalView to ONLINE before it
finishes writing the committed ZNRecord to ZK. During this window, the segment
is not CONSUMING in EV (so it passes the EV check), the ZNRecord exists but
still has `startTime = -1`, and `extractIntervalFromSegmentZKMetaZNRecord`
returns `DEFAULT_INTERVAL`. If cached at that moment, the segment is stuck with
`DEFAULT_INTERVAL` forever since cached segments are never re-fetched.
Added an `isCommittedZNRecord` helper `(znRecord != null && startTime >= 0)`
that handles both the null case, applied in both `init()` and
`onAssignmentChange()`.
--
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]
