SEZ9 commented on PR #12755: URL: https://github.com/apache/seatunnel/pull/12755#issuecomment-6432271211
Following up on nzw921rx's request to handle the open points before this moves forward. Comparing head `f7e7e34240b7` with the previously reviewed `c6b072cd35b9`, the only delta is the `dev` merge; the production change, both test classes and the docs hunks are unchanged, so the earlier points are still open. To be clear about where I stand: the main-path fix (the lite member keeps retrying/re-discovering instead of deadlocking until `hazelcast.max.join.seconds`) is correct and at the right layer, but the retry branch's exit conditions still need work before merge. Remaining asks, in priority order: 1. **F1 (blocker)** – Guard the lite-member retry branch so it only fires when there is actually something to retry. Today it also triggers for an empty candidate list and for an all-permanent blacklist, logs "temporarily blacklisted, will keep retrying", and then burns the whole join window. Please either skip the branch (falling through to the existing failure path) in those cases, or make the log/behaviour reflect what is really happening. My earlier assumption that interrupt handling is safe because the next `tryJoinAddresses()` reaches `blockOnJoin` does not hold here, since `tryJoinAddresses()` never gets that far with no candidates — please give the retry loop its own interrupt exit. 2. **F2** – The `continue` bypasses `clusterService.setMasterAddressToJoin(null)`, so a stale master address keeps the loop pinned to a dead master. Please reset it before `continue`. 3. **F3** – An empty candidate list satisfies `isAllBlacklisted`; for discovery-based workers, re-run discovery on each retry cycle rather than sleeping the whole window. 4. **F4** – Please avoid re-emitting Hazelcast's INFO "added to the blacklist" / "removed from the blacklist" pair for every candidate on every cycle (e.g. only touch entries that actually changed, or downgrade our own per-cycle log). 5. **F5** – Add a short note to the separated-cluster deployment docs (en/zh) that a worker may now be started before the master. 6. **F6** – In the started-before-master tests, make sure the still-starting worker node is shut down on the timeout path so it cannot leak into later tests in the same JVM. 7. **F7** – Sync the PR description and the test Javadoc with the tests that actually exist, and make the scenario numbering consistent. Once F1–F3 are addressed I'm happy to take another full pass; F4–F7 can land in the same push. Thanks for the work so far — the core idea is right, it just needs the edge-case exits tightened. <!-- streview-comment:2067 --> -- 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]
