Jackie-Jiang commented on PR #18855: URL: https://github.com/apache/pinot/pull/18855#issuecomment-5999738640
IMO the limitation here is that remote routing does not receive the segment-metadata refresh notification used by local brokers. We should avoid changing the default behavior of `SegmentZkMetadataFetcher` and its listeners to work around that limitation. The existing separation is useful: assignment changes discover/remove segments, and `refreshSegment()` explicitly updates metadata for an existing segment. Consuming segments must still have their metadata fetched and retained because it already contains partition information needed for pruning/routing. The latest revision restores the initial fetch, but `_segmentsSeen`, completed-status checks, and ExternalView filtering now mix segment lifecycle tracking into this shared module. Assignment callbacks also start carrying metadata replacements that different listeners handle differently. I would suggest two options: 1. **Preferred: fix refresh delivery in remote routing.** Observe completion of the actual segment metadata and invoke the existing `refreshSegment()` path. A metadata watch or an independent periodic check of pending segments could provide this. An ExternalView transition alone is insufficient: with pauseless ingestion, ONLINE can precede final DONE metadata, and another assignment change is not guaranteed afterward. With reliable refresh delivery, `TimeSegmentPruner.refreshSegment()` already replaces the interval, so the special update behavior in `onAssignmentChange()` should not be necessary. 2. **If that cannot be made to work, introduce an optional pluggable implementation.** Extract a `SegmentZkMetadataFetcher` interface, preserve the existing implementation/behavior as the default, and allow remote routing to select a custom implementation. Its fetching, caching, retry, and refresh semantics should be explicit and opt-in. The custom implementation's maintainers would own the correctness and performance consequences of those semantics, rather than changing the default contract for every broker. For the pluggable option, the shared listener contract still needs to remain stable: deliver initial metadata for consuming segments and use the expected refresh callbacks for metadata replacements. If a custom implementation requires different listener semantics, those changes need to be isolated with that implementation too. Please validate the chosen approach with a remote-routing test that delivers final metadata after the last assignment change, while keeping partition metadata available throughout consumption. -- 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]
