SEZ9 commented on PR #12755: URL: https://github.com/apache/seatunnel/pull/12755#issuecomment-6229123387
Thanks for the follow-up in `19a3592c` (on top of `c0d2398c`; my previous pass was on `a5734200`). The updated lite-member branch — one `JOIN_RETRY_WAIT_TIME` wait, clearing only temporary blacklist entries, resetting the master address to join when something was actually cleared, and then `continue` — addresses the stale-master pinning concern (F2), and the `retryStateLogged` guard should stop our own "will keep retrying" message from repeating every cycle. Nice. A few things I'd still like to confirm or see before approving: 1. **F1 / F3 – empty candidate list and all-permanent blacklist.** Since `isAllBlacklisted(possibleAddresses)` is still the entry condition, what happens when `possibleAddresses` is empty or every entry is permanently blacklisted? If `clearedAny` is false there, does the loop still sleep through `hazelcast.max.join.seconds` with nothing to retry? Please either point me at how that path now bails out (or re-runs discovery for the discovery-based joiner), or add a guard so we don't log "temporarily blacklisted" when nothing temporary was found. 2. **F4 – Hazelcast's own blacklist log lines.** The guard covers our message, but each retry cycle still unblacklists every candidate, so I'd expect Hazelcast's INFO "added to the blacklist" / "removed from the blacklist" pair per address per cycle until the master appears. Could you confirm whether that happens on this head, and if so whether it is acceptable or can be reduced? 3. **F5 – docs.** The worker-before-master startup ordering is a user-visible behavior change; please add a short note to the separated-cluster deployment docs (en and zh) if it isn't already in this head. 4. **F6 – test cleanup.** For the started-before-master tests, please make sure the still-starting worker node is shut down on the timeout path so it can't leak into later tests in the same JVM. 5. **F7 – description / Javadoc sync.** Please update the PR description and the test Javadoc so they match the tests actually on disk (the listed-but-missing test and the scenario numbering). Nothing else from my side — once the above is clarified or addressed I'm happy to take another look. <!-- streview-comment:1828 --> -- 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]
