DanielLeens commented on PR #12755: URL: https://github.com/apache/seatunnel/pull/12755#issuecomment-6359847814
Thanks @SEZ9 for the four open points, and thanks @WXPNG for the sync. I re-checked each of them against head `4c3f68db66` (static reading, nothing executed locally) and also looked at the CI result, which is red. **CI first.** The apache-side `Build` failure points to the fork run [39770755603](https://github.com/WXPNG/seatunnel/actions/runs/39770755603) (attempt 2). The lanes that failed are `hbase-connector-it` (11/17), `mysql-cdc-connector-it` (11/17), `all-connectors-it-2` (11/17), `all-connectors-it-6` (11/17) and `transform-v2-it-part-2` (11/17). I read samples of their logs: `HbaseIT` errors out for all 22 cases within about 27s, and `TestSparkDateTimeTransformIT` and `FlinkMetricsIT` fail with `PreconditionViolation` (no supporting container is available). None of these touches `seatunnel-engine-server` or the joiner. On the other side, all four `unit-test` jobs, `engine-v2-it`, `engine-k8s-it` and `engine-resource-managers-it` are green, and the `unit-test (11, ubuntu-latest)` log shows `LiteNodeDropOutTcpIpJoinerTest` (4 tests) and `LiteNodeDropOutDiscoveryJoinerTest` (2 tests) passing with 0 failures. So the failure does not come from this diff. Minimal next step: in the fork, u se "Re-run failed jobs" (job-level, not the whole workflow). Please do not add unrelated patches to chase these lanes. If the same lanes fail again, post the job links here and we will look at them one by one. **SEZ9's points, on this head:** 1. **All-permanent blacklist.** In `joinViaPossibleMembers` (`LiteNodeDropOutTcpIpJoiner.java`, the `isAllBlacklisted` branch) a lite member with only permanent entries gets `clearedAny == false` from `retryTemporaryBlacklistedAddresses`, logs the WARN `All candidate master addresses are permanently blacklisted` once, and keeps looping once per `JOIN_RETRY_WAIT_TIME` until `hazelcast.max.join.seconds`. That is the same end result as before the PR (a lite member can never self-elect, so there is nothing else it could do), it just no longer claims anything is being retried. The new control test `retryMustNotClearAPermanentBlacklistEvenWhenTheMasterIsReachable` pins that the retry never clears a permanent entry. I do not see a regression here. 2. **Blacklist log churn.** Not emitted at INFO on this head for the retry case: the overrides of `blacklist(Address, boolean)` and `unblacklist(Address)` log at FINE when the node is a lite member and the entry is temporary, and only then. Non-lite members and permanent entries still go through `super` and keep Hazelcast's INFO. The Javadoc also tells maintainers to re-check the two overrides when the shaded Hazelcast is upgraded. 3. **Worker cleanup in the tests.** The two started-before-master tests and the empty-discovery test do `workerThread.interrupt()` plus a bounded `join(5s)` and `HazelcastInstanceFactory.shutdownAll()` in `finally`. `workerWithoutMasterShouldTimeoutAndNeverBecomeMaster` has no `finally`, because the node never finishes starting (the `IllegalStateException` comes out of `startNode`); that is an inference from Hazelcast's startup-failure handling, not something I observed. If you want to be defensive, a `HazelcastInstanceFactory.shutdownAll()` in an `@AfterEach` for the class would make that explicit, but I treat it as a small suggestion, not a blocker. 4. **PR description vs tests.** I compared the `Tests` section of the description with the test sources: the 4 methods of `LiteNodeDropOutTcpIpJoinerTest` and the 2 methods of `LiteNodeDropOutDiscoveryJoinerTest` are all listed, no listed test is missing, and the test Javadoc no longer uses "Scenario N" numbering. Nothing to remove. My source-level conclusion is unchanged: no open source issue. The head is the dev-merge `4c3f68db66`, so I am not posting another full review. The merge gate is still a green fork `Build`; and @SEZ9's threads stay his to resolve after his own re-read. -- 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]
