vbhanuchander-lang commented on PR #309: URL: https://github.com/apache/flink-connector-kafka/pull/309#issuecomment-5617636889
Thanks @MartijnVisser — that is a thorough review, and the correction on the rationale is the important one. All six points addressed, force-pushed on top of `main` (`91e01a5`, so #289's `ReaderRecoveryGate` is in). **Rationale reworded in both the description and the commit message.** You are right and I was wrong: I had claimed `addSplitsBack` is never called for a reader that owned nothing, so resetting there would strand an idle reader. `SourceCoordinator.subtaskReset` calls it for *every* reset subtask, with an empty list in that case, which is exactly why the empty call re-signals everyone on `main`. Both now say what actually holds — registration is the event that makes a new attempt addressable, so it is what has to re-arm the signal. **`handleNoMoreSplits` simplified** to `if (readersSignalledNoMoreSplits.add(readerId))` per reader; the temporary set and the early `return` are gone. **Comment on the metadata path** now records that `onHandleSubscribedStreamsFetch` recreates the sub-enumerators and their assignments would need a fresh signal, that FLINK-31006 means that path does not signal at all today, and that clearing the set there is therefore not observable yet. Left as a follow-up as you suggested, so this set does not become the thing that blocks it once 31006 is fixed. **Test strengthened** using your ordering — `unregisterReader` before `addSplitsBack`, assert nobody moves off 1, then re-register and assert the reset reader reaches 2 while the others stay at 1. That asserts the half of the contract I had left uncovered. **`spotless`** fixed. That was my mistake in a way worth owning: I ran `spotless:check` before opening the PR and read the exit code of a `tail` in the pipeline rather than Maven's, so I reported it clean when it was not, and every CI leg died on it. Now verified properly — `spotless:check` and `checkstyle:check` both exit 0, and the class is 31/31 locally. **Duplicate helper removed** — `createEnumerator` takes a `Boundedness` parameter and picks the matching stopping offsets initializer; the near-copy is gone. **Ordering** noted in the description: this should go in after #291. Your numbers make the point clearly — 1 of 24 with this change alone versus 0 of 20 with both — so this closes the re-signal cause and #291 closes the reader-side hang. Happy to wait for #291 and rebase, or to have them go in together, whichever you prefer. On the generative tooling disclosure: added `Generated-by: Claude Code (Opus 5)` to both the description and the commit trailer, per https://www.apache.org/legal/generative-tooling.html. Thanks for pointing at the apache/flink template — I had not realised this repo inherits it. -- 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]
