davsclaus commented on code in PR #27613:
URL: https://github.com/apache/camel/pull/27613#discussion_r4236603840
##########
components/camel-jgroups-raft/src/main/java/org/apache/camel/component/jgroups/raft/cluster/ClusterRoleChangeListener.java:
##########
@@ -46,14 +46,17 @@ public void roleChanged(Role role) {
}
break;
case Follower:
+ case Learner:
Review Comment:
+1. This matches the library contract: `Learner extends Follower`, and
`RAFT._removeServer()` moves a removed leader directly from Leader to Learner.
So this case is what clears `isMaster` when that happens, and on main that path
left a stale master.
##########
components/camel-jgroups-raft/src/main/java/org/apache/camel/component/jgroups/raft/cluster/JGroupsRaftClusterView.java:
##########
@@ -83,12 +83,15 @@ protected void doStart() throws Exception {
}
fireLeadershipChangedEvent((CamelClusterMember) null);
+ // Register the listener once before attempting to connect;
registering inside
+ // the retry loop would accumulate duplicate listeners across retries.
+ raftHandle.addRoleListener(new ClusterRoleChangeListener(this));
Review Comment:
Fine to hoist this. Two small notes:
- Duplicate listeners never caused duplicate `fireLeadershipChangedEvent()`
calls. Every listener checks `isMaster()` before firing, and RAFT calls them
one after another on the same thread. So the "N duplicate events" wording in
the commit message overstates it. It was only extra listener objects.
- A stop/start of the view (where `raftHandle` is kept, because only
`doShutdown` nulls it) still adds a new listener on every `doStart`. If you
want to fully close this, you could keep the listener in a field and call
`raftHandle.removeRoleListener(listener)` in `doStop()`. Optional.
##########
components/camel-jgroups-raft/src/test/java/org/apache/camel/component/jgroups/raft/cluster/ClusterRoleChangeListenerTest.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.component.jgroups.raft.cluster;
+
+import java.util.List;
+import java.util.Optional;
+
+import org.apache.camel.cluster.CamelClusterMember;
+import org.jgroups.protocols.raft.Role;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Unit tests for {@link ClusterRoleChangeListener} that verify role
transitions without requiring a real JGroups
+ * cluster.
+ */
+public class ClusterRoleChangeListenerTest {
+
+ private JGroupsRaftClusterView view;
+ private ClusterRoleChangeListener listener;
+
+ @BeforeEach
+ void setUp() {
+ // Minimal in-memory view: no cluster service needed, no listeners
registered,
+ // so fireLeadershipChangedEvent() is a safe no-op.
+ view = new JGroupsRaftClusterView(null, "test-ns", null, null, null,
"A") {
Review Comment:
The anonymous subclass is not needed. `new JGroupsRaftClusterView(null,
"test-ns", null, null, null, "A")` works as is, because `getLocalMember()` only
returns the inner member and `getId()` is never called on this path. Overriding
`getLeader()`/`getLocalMember()` also swaps out the production methods that
`ClusteredRoutePolicy` depends on. Using the real view would let the tests
assert `view.getLeader().isEmpty()` / `view.getLocalMember().isLeader()`. You
could also register a `CamelClusterEventListener.Leadership` via
`view.addEventListener(...)` and assert that the `null` leadership event fires
on Learner. That event is what actually stops the clustered route. The same
applies to `JGroupsRaftClusterViewTest`.
##########
components/camel-jgroups-raft/src/test/java/org/apache/camel/component/jgroups/raft/cluster/ClusterRoleChangeListenerTest.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.component.jgroups.raft.cluster;
+
+import java.util.List;
+import java.util.Optional;
+
+import org.apache.camel.cluster.CamelClusterMember;
+import org.jgroups.protocols.raft.Role;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Unit tests for {@link ClusterRoleChangeListener} that verify role
transitions without requiring a real JGroups
+ * cluster.
+ */
+public class ClusterRoleChangeListenerTest {
+
+ private JGroupsRaftClusterView view;
+ private ClusterRoleChangeListener listener;
+
+ @BeforeEach
+ void setUp() {
+ // Minimal in-memory view: no cluster service needed, no listeners
registered,
+ // so fireLeadershipChangedEvent() is a safe no-op.
+ view = new JGroupsRaftClusterView(null, "test-ns", null, null, null,
"A") {
+ @Override
+ public Optional<CamelClusterMember> getLeader() {
+ return isMaster() ? Optional.of(getLocalMember()) :
Optional.empty();
+ }
+
+ @Override
+ public CamelClusterMember getLocalMember() {
+ return new CamelClusterMember() {
+ @Override
+ public boolean isLeader() {
+ return isMaster();
+ }
+
+ @Override
+ public boolean isLocal() {
+ return true;
+ }
+
+ @Override
+ public String getId() {
+ return "A";
+ }
+ };
+ }
+
+ @Override
+ public List<CamelClusterMember> getMembers() {
+ return List.of(getLocalMember());
+ }
+ };
+ listener = new ClusterRoleChangeListener(view);
+ }
+
+ @Test
+ void leaderRoleSetsViewAsMaster() {
+ assertFalse(view.isMaster());
+ listener.roleChanged(Role.Leader);
+ assertTrue(view.isMaster());
+ }
+
+ @Test
+ void followerRoleClearsViewMasterStatus() {
+ view.setMaster(true);
+ listener.roleChanged(Role.Follower);
+ assertFalse(view.isMaster());
+ }
+
+ @Test
+ void learnerRoleClearsViewMasterStatus() {
+ view.setMaster(true);
+ // Role.Learner previously fell through to default: throw, crashing
the listener.
Review Comment:
Nit: the listener was never crashed, because RAFT swallows the exception.
This comment would match the corrected commit message:
```suggestion
// Role.Learner previously fell through to default: throw (swallowed
by RAFT), leaving the view as master.
```
--
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]
