Jackie-Jiang commented on PR #18855: URL: https://github.com/apache/pinot/pull/18855#issuecomment-5723109954
Consuming segments should still be fetched and cached because they already contain partition metadata for partitioned tables. The controller writes that metadata when creating the consuming segment, and both partition pruning and `SegmentPartitionMetadataManager` need it before the segment commits. Missing final time bounds should leave the segment at `DEFAULT_INTERVAL` in the time pruner. The local commit callback already supports this lifecycle: after persisting the final metadata, the controller sends a segment-refresh message, and `SegmentZkMetadataFetcher.refreshSegment()` unconditionally re-fetches metadata and updates all listeners, regardless of whether the segment is cached. The gap to address is the equivalent refresh for remote routing, which does not receive that message. The [new skip in `onAssignmentChange()`](https://github.com/apache/pinot/blob/8c852055389243c7f8243a563950092fc162de13/pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java#L102-L116) changes behavior for local routing too. If a new segment is first observed after initialization and is already CONSUMING in the ExternalView, it never reaches the listeners: - `TimeSegmentPruner` never inserts its default interval, so time-filtered queries omit the segment's rows until commit or refresh. - `SegmentPartitionMetadataManager` creates an entry with an invalid partition ID despite valid partition metadata being available. This can make partition-hinted realtime queries fail. The new time-pruner test starts the consuming segment in `init()`, which still fetches its metadata, so it does not cover this regression. There is also an event-ordering gap: `onlineSegments` comes from IdealState, and assignment callbacks run for both IdealState and ExternalView changes. A new consuming segment can be discovered before its ExternalView entry exists. The helper then returns false, allowing unfinished metadata to be cached permanently and leaving the original remote-pruning problem unresolved. I'd prefer preserving the initial fetch/cache behavior and adding an explicit remote metadata-refresh mechanism that reuses `refreshSegment()`. For example, watch metadata for unfinished realtime segments and refresh when it becomes DONE, then remove the watch. Registering the watch followed by a re-read would close the concurrent-commit race. Periodic batch refresh of pending segments is another option, with extra reads and delayed pruning. A one-time refresh on CONSUMING → ONLINE is insufficient for pauseless ingestion: [IdealState becomes ONLINE while metadata is still COMMITTING](https://github.com/apache/pinot/blob/8c852055389243c7f8243a563950092fc162de13/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java#L889-L955), and final metadata is written later. Completion should therefore be confirmed from segment metadata. Please cover post-init discovery of a consuming segment, partition metadata availability while consuming, discovery before the ExternalView entry exists, and the pauseless COMMITTING → DONE sequence. -- 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]
