rzo1 commented on PR #9153: URL: https://github.com/apache/storm/pull/9153#issuecomment-6387517555
Thanks for the update and for scoping the ReadClusterState catch, that part looks good to me. I agree with @GGraziadei's change request. The new `ConnectionAwareRetryPolicyEventThreadTest` failure is deterministic, not flaky: `allowRetry` calls `blockUntilConnected` on every thread, including the ZooKeeper EventThread. That is the thread that delivers the reconnect, so the wait always runs to the full session timeout. For Curator background operations (e.g. the `LeaderLatch` used by Nimbus), this could turn a short blip into a session expiry. A few suggestions: 1. Only wait for foreground retries, e.g. `sleeper == RetryLoop.getDefaultRetrySleeper()`, as @reiabreu suggested on Oct 3. Alternatively, and my preference, move `ConnectionAwareRetryPolicy`/`CuratorUtils` to a follow-up PR and keep this one supervisor-only. The two catches already fix the crash. 2. Narrow the `SupervisorHeartbeat` catch to ZooKeeper failures (e.g. a `KeeperException` cause) and rethrow everything else. The comment there is also not accurate: this write is what recreates the ephemeral `/supervisors/<id>` node after a LOST, so a persistent error would leave the supervisor invisible to Nimbus. 3. Please update the PR description. It still mentions the removed `nimbus.supervisor.timeout.secs` reasoning. -- 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]
