suvodeep-pyne opened a new pull request, #19995: URL: https://github.com/apache/pinot/pull/19995
## Summary Follow-up to #19740 for [Jackie's post-merge review](https://github.com/apache/pinot/pull/19740#discussion_r4331476685). When the IdealState update of a pauseless commit start fails, #19740 keeps the new segment's ZK metadata for the next attempt. If the table is paused before that attempt (or the topic is paused, or the partition leaves the stream), the retry completes the commit without a new segment, but the kept segment stays. `RealtimeSegmentValidationManager` later takes it as the latest segment of the partition and adds it to the IdealState. For example, resuming the paused table with `consumeFrom=largest` then starts that partition from the committed end offset instead. ## Changes - A resumed commit start looks for the earlier attempt's new segment before step 2 decides whether the partition still needs a new segment. `createNewSegmentMetadata` takes that segment instead of the `resumed` flag (Jackie's second suggestion). It uses the segment only after the existing checks pass, i.e. the partition is still in the stream and the table/topic is not paused, so finding it never by itself gets it assigned. - After step 3, if the commit completed without the earlier attempt's segment and that segment is not in the IdealState, its ZK metadata is removed. The committing segment is ONLINE by then, so no attempt of the commit can add the segment anymore. Not changed: - If no retry succeeds before the max segment completion time, the kept segment stays, and the validation manager adds it, as it does after a controller stop between steps 2 and 3. Removing it in the failure handler while the table is paused could race with a resume and a resumed attempt on another controller, and leave a CONSUMING segment without ZK metadata. - The new cleanup removes the metadata without a fence against the IdealState, like the existing cleanups in the same method. An attempt that stalls between its IdealState update and the removal could remove a segment of the same name that validation created in the meantime, which needs a resume and a repair within the same minute as the first attempt. - Pauseless tables only support a single stream, and validation does not skip a paused topic of a single-stream table, so for topic pause the change only removes the kept segment. - Jackie's first suggestion (`prepareCommitStart`) would change step 1 of every pauseless commit to save a few reads on the resume path. I kept it out of this fix. ## Testing New tests in `PinotLLCRealtimeSegmentManagerTest`. Each one fails without the fix: - The table is paused between the failed attempt and the retry. The retry removes the kept segment, and resuming the table with an offset criteria creates a new segment from the selected offset. - The topic is paused between the failed attempt and the retry. The retry removes the kept segment. - The partition is removed from the stream between the failed attempt and the retry. The retry completes without adding the kept segment and removes it. Existing tests pass: `PinotLLCRealtimeSegmentManagerTest`, `SegmentCompletionTest`, `SegmentCompletionFSMFactoryTest`, `RealtimeSegmentValidationManagerTest`, and `PauselessRealtimeIngestionIntegrationTest`. End to end, I ran a local cluster test (not part of this PR) with ZooKeeper, Kafka, a controller, a broker and a server. Every commit start fails before the IdealState update. The test then pauses one table and the topic of another, removes the failure so that the servers' retries succeed, and resumes the first table with `consumeFrom=largest`: | | Kept segments after the retries | Resume from `largest` | |---|---|---| | Without the fix | Remain in both tables | Adds the kept segments, which start at the committed end offset (2500); the table ingests all 115545 rows | | With the fix | Removed in both tables | New segments start at the end of the topic; the count stays at 5000 | -- 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]
