From 6fbaf5647c8ad3f6476b80dc0ae8f2b11508de89 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Fri, 21 Mar 2025 17:29:03 +0100 Subject: [PATCH] Improve AbstractLeader class hierarchy AbstractLeader.handleAppendEntriesReply() always results in 'this' being returned and is overridden in the three subclasses. Rename it to processAppendEntriesReply(), without the ability to change behavior and make it final. The subclasses then use it as a common utility, doing their own thing as needed. Since Leader is subclassed in tests, we lock down all its methods except the single one that is being overridden. Change-Id: I594bebedfa612f7e946d97040ea55c053c2c9f3d Signed-off-by: Robert Varga --- .../cluster/raft/behaviors/AbstractLeader.java | 10 ++++------ .../cluster/raft/behaviors/IsolatedLeader.java | 15 ++++++++------- .../controller/cluster/raft/behaviors/Leader.java | 14 +++++++------- .../cluster/raft/behaviors/PreLeader.java | 7 +++++++ 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java index 8df9b7e73e..30851de21a 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java @@ -205,8 +205,8 @@ public abstract sealed class AbstractLeader extends RaftActorBehavior permits Is return this; } - @Override - RaftActorBehavior handleAppendEntriesReply(final ActorRef sender, final AppendEntriesReply appendEntriesReply) { + // handleAppendEntriesReply() without switching behaviors + final void processAppendEntriesReply(final ActorRef sender, final AppendEntriesReply appendEntriesReply) { LOG.trace("{}: handleAppendEntriesReply: {}", logName, appendEntriesReply); // Update the FollowerLogInformation @@ -215,14 +215,14 @@ public abstract sealed class AbstractLeader extends RaftActorBehavior permits Is if (followerLogInformation == null) { LOG.error("{}: handleAppendEntriesReply - unknown follower {}", logName, followerId); - return this; + return; } final var followerRaftVersion = appendEntriesReply.getRaftVersion(); if (followerRaftVersion < RaftVersions.FLUORINE_VERSION) { LOG.warn("{}: handleAppendEntriesReply - ignoring reply from follower {} raft version {}", logName, followerId, followerRaftVersion); - return this; + return; } final long lastActivityNanos = followerLogInformation.nanosSinceLastActivity(); @@ -334,8 +334,6 @@ public abstract sealed class AbstractLeader extends RaftActorBehavior permits Is //Send the next log entry immediately, if possible, no need to wait for heartbeat to trigger that event sendUpdatesToFollower(followerId, followerLogInformation, false, !updated); - - return this; } // Invoked after persistence is complete to check if replication consensus has been reached. diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/IsolatedLeader.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/IsolatedLeader.java index 44971ad5ed..a3fc50ec53 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/IsolatedLeader.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/IsolatedLeader.java @@ -42,14 +42,15 @@ public final class IsolatedLeader extends AbstractLeader { // we received an Append Entries reply, we should switch the Behavior to Leader @Override RaftActorBehavior handleAppendEntriesReply(final ActorRef sender, final AppendEntriesReply appendEntriesReply) { - final var ret = super.handleAppendEntriesReply(sender, appendEntriesReply); + processAppendEntriesReply(sender, appendEntriesReply); - // it can happen that this isolated leader interacts with a new leader in the cluster and - // changes its state to Follower, hence we only need to switch to Leader if the state is still Isolated - if (ret.state() == RaftState.IsolatedLeader && !isLeaderIsolated()) { - LOG.info("{}: IsolatedLeader {} switching from IsolatedLeader to Leader", getId(), getLeaderId()); - return switchBehavior(new Leader(context, this)); + // it can happen that this isolated leader interacts with a new leader in the cluster and changes its state to + // Follower, hence we only need to switch to Leader if the state is still Isolated + if (isLeaderIsolated()) { + return this; } - return ret; + + LOG.info("{}: IsolatedLeader {} switching from IsolatedLeader to Leader", getId(), getLeaderId()); + return switchBehavior(new Leader(context, this)); } } diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Leader.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Leader.java index 6166efd909..6181a09d98 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Leader.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Leader.java @@ -91,7 +91,7 @@ public non-sealed class Leader extends AbstractLeader { } @Override - protected void beforeSendHeartbeat() { + protected final void beforeSendHeartbeat() { if (isolatedLeaderCheck.elapsed(TimeUnit.MILLISECONDS) > context.getConfigParams().getIsolatedCheckIntervalInMillis()) { context.getActor().tell(ISOLATED_LEADER_CHECK, context.getActor()); @@ -106,10 +106,10 @@ public non-sealed class Leader extends AbstractLeader { } @Override - RaftActorBehavior handleAppendEntriesReply(final ActorRef sender, final AppendEntriesReply appendEntriesReply) { - RaftActorBehavior returnBehavior = super.handleAppendEntriesReply(sender, appendEntriesReply); + final Leader handleAppendEntriesReply(final ActorRef sender, final AppendEntriesReply appendEntriesReply) { + processAppendEntriesReply(sender, appendEntriesReply); tryToCompleteLeadershipTransfer(appendEntriesReply.getFollowerId()); - return returnBehavior; + return this; } /** @@ -181,7 +181,7 @@ public non-sealed class Leader extends AbstractLeader { } @Override - public void close() { + public final void close() { if (leadershipTransferContext != null) { LeadershipTransferContext localLeadershipTransferContext = leadershipTransferContext; leadershipTransferContext = null; @@ -192,12 +192,12 @@ public non-sealed class Leader extends AbstractLeader { } @VisibleForTesting - void markFollowerActive(final String followerId) { + final void markFollowerActive(final String followerId) { getFollower(followerId).markFollowerActive(); } @VisibleForTesting - void markFollowerInActive(final String followerId) { + final void markFollowerInActive(final String followerId) { getFollower(followerId).markFollowerInActive(); } diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/PreLeader.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/PreLeader.java index c3a47770e3..9fde56327d 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/PreLeader.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/PreLeader.java @@ -11,6 +11,7 @@ import org.apache.pekko.actor.ActorRef; import org.opendaylight.controller.cluster.raft.RaftActorContext; import org.opendaylight.controller.cluster.raft.RaftState; import org.opendaylight.controller.cluster.raft.base.messages.ApplyState; +import org.opendaylight.controller.cluster.raft.messages.AppendEntriesReply; import org.opendaylight.controller.cluster.raft.persisted.NoopPayload; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -52,4 +53,10 @@ public final class PreLeader extends AbstractLeader { return super.handleMessage(sender, message); } } + + @Override + PreLeader handleAppendEntriesReply(final ActorRef sender, final AppendEntriesReply appendEntriesReply) { + processAppendEntriesReply(sender, appendEntriesReply); + return this; + } } -- 2.36.6