davsclaus commented on code in PR #25611:
URL: https://github.com/apache/camel/pull/25611#discussion_r3854809208
##########
components/camel-zookeeper/src/main/java/org/apache/camel/component/zookeeper/cluster/ZooKeeperClusterView.java:
##########
@@ -110,6 +111,7 @@ protected void doStart() throws Exception {
if (leaderSelector == null) {
leaderSelector = new LeaderSelector(client, getFullPath(), new
CamelLeaderElectionListener());
leaderSelector.setId(getClusterService().getId());
+ leaderSelector.autoRequeue();
Review Comment:
`autoRequeue()` puts this `LeaderSelector` instance into "always requeue
after `takeLeadership()` exits" mode for its whole lifetime, not just for the
ZK-disconnect case. `doStop()` calls `interruptLeadership()` without closing
the selector (`close()` only happens in `doShutdown()`) — could a deliberate
`doStop()` (as opposed to a ZK session loss) cause this node to automatically
re-enter the leader race and reclaim leadership before `doShutdown()` runs?
Worth double-checking the stop → shutdown sequencing to rule out unintended
re-election on a normal stop.
##########
components/camel-zookeeper/src/main/java/org/apache/camel/component/zookeeper/cluster/ZooKeeperClusterView.java:
##########
@@ -119,8 +121,9 @@ protected void doStart() throws Exception {
@Override
protected void doStop() throws Exception {
if (leaderSelector != null) {
+ leader = false;
leaderSelector.interruptLeadership();
- fireLeadershipChangedEvent(getLeader().orElse(null));
+ fireLeadershipChangedEvent((CamelClusterMember) null);
Review Comment:
Combined with the `try/finally` change in `takeLeadership()`: calling
`interruptLeadership()` here unblocks the thread inside `takeLeadership()`,
which runs its own `finally` block (another `leader = false` +
`fireLeadershipChangedEvent(null)`), and then falls through to the leftover
line flagged in the other comment (a third fire). So a single `doStop()` call
can now emit the same "leadership lost" notification up to three times.
Consider whether `doStop()` still needs to fire the event itself now that
`takeLeadership()`'s `finally` block always does it, or gate one of the two
paths.
##########
components/camel-zookeeper/src/main/java/org/apache/camel/component/zookeeper/cluster/ZooKeeperClusterView.java:
##########
@@ -142,15 +145,20 @@ private String getFullPath() {
private final class CamelLeaderElectionListener extends
LeaderSelectorListenerAdapter {
@Override
public void takeLeadership(CuratorFramework curatorFramework) throws
Exception {
+ leader = true;
fireLeadershipChangedEvent(localMember);
BlockingTask task =
Tasks.foregroundTask().withBudget(Budgets.iterationBudget()
.withMaxIterations(IterationBoundedBudget.UNLIMITED_ITERATIONS)
.withInterval(Duration.ofSeconds(5))
.build())
.build();
-
- task.run(getCamelContext(), () -> !isRunAllowed());
+ try {
+ task.run(getCamelContext(), () -> !isRunAllowed() ||
!leaderSelector.hasLeadership());
+ } finally {
+ leader = false;
+ fireLeadershipChangedEvent((CamelClusterMember) null);
+ }
fireLeadershipChangedEvent(getLeader().orElse(null));
Review Comment:
This statement is left over from before the `try/finally` above was
introduced. The `finally` block already fires the leadership-lost event (with
`leader = false`) whenever `takeLeadership()` exits — whether normally or via
interruption. This line now fires a **second**, redundant leadership-changed
event immediately afterward, using whatever `getLeader()` currently returns
(which, in the exact disconnect scenario this PR fixes, may already be a
different node that won the election on the healthy side). Recommend deleting
this line so only the `finally` block's event fires.
--
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]
