shauryachats commented on PR #18855: URL: https://github.com/apache/pinot/pull/18855#issuecomment-5273899525
> Do we need to change `SegmentZkMetadataFetcher`? I think we do want to cache all segments We do want to eventually cache every segment - the issue is when. The old `znRecord != null` check caches a segment the first time we see it, but a CONSUMING segment already has a non-null ZNRecord (just `startTime=-1`), so it gets cached prematurely and is never re-fetched once `_onlineSegmentsCached` contains it. For local brokers this was masked by the direct `refreshSegment()` UDM path on commit, but remote/spectator brokers (this PR's target) never receive that message - `onAssignmentChange` via the EV watch is their only update path, so caching before commit means they get stuck at `DEFAULT_INTERVAL` forever (meaning no effective time pruning). The EV-state check just delays caching until the segment is actually committed, so it still ends up cached - just not before it has real data. -- 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]
