SEZ9 commented on PR #11900: URL: https://github.com/apache/seatunnel/pull/11900#issuecomment-5552599385
Thanks @DanielLeens for the thorough end-to-end re-review of `bf36ed7d56a` — much appreciated, especially the correctly-scoped three-way diff against `dev` after the rebase. Answering your points directly: 1. **Indentation regression in `clearCoordinatorService()`** — good catch, that was indeed a sloppy conflict resolution on my side, not intentional. I'll fix the indentation on that statement and push; agreed it's not a nitpick given it's the line CI tripped on. 2. **F2 (shutdown ordering)** — glad the fix is re-confirmed: `jobHistoryService.shutdown()` stays ahead of the rethrowing `eventProcessor.close()` block, so listeners are deregistered even if the later steps throw. 3. **F1 (PR scope vs. claimed feature)** — the actual scope of this PR is the listener-leak fix on master-role switch, exactly as you summarized it in "What Problem Does This PR Solve". I'll update the PR title/description to match that scope so the claim and the diff line up. Please let me know if that resolves F1 for you or if you'd rather see the description reworded differently. 4. **F3 (test coverage)** — you're right that `JobHistoryServiceTest.java` is unchanged from last round, so this is still open. Before I write it: would a unit test asserting that `shutdown()` invokes `removeEntryListener` with the captured UUIDs on all three IMaps satisfy this, or do you want coverage that simulates an actual master-role switch through `clearCoordinatorService()`? I'd like to align on the expected level before pushing. 5. **F4 (ignored boolean from `removeEntryListener`)** — still open as well. My plan is to check the boolean result in `removeEntryListenerQuietly()` and log a warning when removal returns false, keeping the "quiet" (non-throwing) behavior. If you'd prefer something stronger than a warning, say the word. Remaining work on my side: fix the indentation, update the PR description, add the F3 test (pending your answer on scope), and handle the F4 return value. I'll push those together in the next commit. <!-- streview-comment:797 --> -- 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]
