SEZ9 commented on PR #11486: URL: https://github.com/apache/seatunnel/pull/11486#issuecomment-5390959836
Thanks @DanielLeens for re-verifying against the actual code — much appreciated. **Issue 3**: withdrawn as originally stated. I missed that `fetch()` calls `unassignPartitions(finishedPartitions)` before returning, so the finished partition is out of `consumer.assignment()` by the next `fetch()` and there's no re-evaluation or repeated `finishedSplits` reporting. The residual point you identified — the entry never being removed from the `stoppingOffsets` map — is a harmless retention issue. Downgrading to a Low, non-blocking cleanup ask: remove the entry alongside the unassignment. **Issue 1**: fair analysis. Since the loop only iterates `consumer.assignment()` and `handleSplitsChanges()` runs `seekToStartingOffsets()` synchronously before a split is considered assigned, `position(tp)` should resolve locally in normal operation. Reclassifying as a defensive-hardening suggestion — a timeout-bounded position call would be a cheap safety net, but it does not block merge. **Issues 2, 4, 6, 7**: agreed these are non-blocking. The one I'd most like to see before merge is Issue 2 — a test asserting final visible records are still delivered when the split finishes in the same fetch, ideally plus an IT against a real transactional topic, since that's the core behavior this PR fixes. **Issue 5**: agreed — with `currentOffset` only feeding a `LOG.debug` line, cosmetic and Low is the right call. Remaining asks: 1. (Preferred pre-merge) The final-records-delivered test per Issue 2; the transactional-topic IT can follow up separately. 2. Remove the `stoppingOffsets` entry when a partition finishes (revised Issue 3). 3. Optional: bounded position call (Issue 1), close the leaked consumer in the test (Issue 4), docs note (Issue 6), brief Javadoc on the new test method and helper (Issue 7). If item 1 lands, I'm happy with this going in. Thanks again for the thorough push-back — it materially improved the review. <!-- streview-comment:512 --> -- 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]
