From ff957698c85aa1b57cdc6d7a8bca2c9dc96ba1ec Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Fri, 23 Jan 2015 15:57:33 +0100 Subject: [PATCH] Do not leak AtomicLong from FollowerLogInformation Leaking this implementation details allows callers to interact with state outside of the official contract. Disallow that. Change-Id: I880310f2a9692fd11ba2cd33830501d19d0a1b65 Signed-off-by: Robert Varga --- .../cluster/raft/FollowerLogInformation.java | 25 ++++++++----------- .../raft/FollowerLogInformationImpl.java | 16 +++++++----- .../raft/behaviors/AbstractLeader.java | 14 ++++++----- .../cluster/raft/behaviors/LeaderTest.java | 14 ++++++++--- 4 files changed, 39 insertions(+), 30 deletions(-) diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/FollowerLogInformation.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/FollowerLogInformation.java index 4d2bad5c2e..6d0c14e733 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/FollowerLogInformation.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/FollowerLogInformation.java @@ -5,11 +5,8 @@ * terms of the Eclipse Public License v1.0 which accompanies this distribution, * and is available at http://www.eclipse.org/legal/epl-v10.html */ - package org.opendaylight.controller.cluster.raft; -import java.util.concurrent.atomic.AtomicLong; - /** * The state of the followers log as known by the Leader */ @@ -19,13 +16,13 @@ public interface FollowerLogInformation { * Increment the value of the nextIndex * @return */ - public long incrNextIndex(); + long incrNextIndex(); /** * Decrement the value of the nextIndex * @return */ - public long decrNextIndex(); + long decrNextIndex(); /** * @@ -37,45 +34,43 @@ public interface FollowerLogInformation { * Increment the value of the matchIndex * @return */ - public long incrMatchIndex(); + long incrMatchIndex(); - public void setMatchIndex(long matchIndex); + void setMatchIndex(long matchIndex); /** * The identifier of the follower * This could simply be the url of the remote actor */ - public String getId(); + String getId(); /** * for each server, index of the next log entry * to send to that server (initialized to leader * last log index + 1) */ - public AtomicLong getNextIndex(); + long getNextIndex(); /** * for each server, index of highest log entry * known to be replicated on server * (initialized to 0, increases monotonically) */ - public AtomicLong getMatchIndex(); + long getMatchIndex(); /** * Checks if the follower is active by comparing the last updated with the duration * @return boolean */ - public boolean isFollowerActive(); + boolean isFollowerActive(); /** * restarts the timeout clock of the follower */ - public void markFollowerActive(); + void markFollowerActive(); /** * This will stop the timeout clock */ - public void markFollowerInActive(); - - + void markFollowerInActive(); } diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/FollowerLogInformationImpl.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/FollowerLogInformationImpl.java index 9820638ab7..91a5dddb98 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/FollowerLogInformationImpl.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/FollowerLogInformationImpl.java @@ -9,10 +9,9 @@ package org.opendaylight.controller.cluster.raft; import com.google.common.base.Stopwatch; -import scala.concurrent.duration.FiniteDuration; - import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; +import scala.concurrent.duration.FiniteDuration; public class FollowerLogInformationImpl implements FollowerLogInformation { @@ -35,6 +34,7 @@ public class FollowerLogInformationImpl implements FollowerLogInformation { this.followerTimeoutMillis = followerTimeoutDuration.toMillis(); } + @Override public long incrNextIndex(){ return nextIndex.incrementAndGet(); } @@ -49,6 +49,7 @@ public class FollowerLogInformationImpl implements FollowerLogInformation { this.nextIndex.set(nextIndex); } + @Override public long incrMatchIndex(){ return matchIndex.incrementAndGet(); } @@ -58,16 +59,19 @@ public class FollowerLogInformationImpl implements FollowerLogInformation { this.matchIndex.set(matchIndex); } + @Override public String getId() { return id; } - public AtomicLong getNextIndex() { - return nextIndex; + @Override + public long getNextIndex() { + return nextIndex.get(); } - public AtomicLong getMatchIndex() { - return matchIndex; + @Override + public long getMatchIndex() { + return matchIndex.get(); } @Override 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 4993d25f20..0a31cde90f 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 @@ -204,7 +204,7 @@ public abstract class AbstractLeader extends AbstractRaftActorBehavior { int replicatedCount = 1; for (FollowerLogInformation info : followerToLog.values()) { - if (info.getMatchIndex().get() >= N) { + if (info.getMatchIndex() >= N) { replicatedCount++; } } @@ -228,6 +228,7 @@ public abstract class AbstractLeader extends AbstractRaftActorBehavior { return this; } + @Override protected ClientRequestTracker removeClientRequestTracker(long logIndex) { ClientRequestTracker toRemove = findClientRequestTracker(logIndex); @@ -238,6 +239,7 @@ public abstract class AbstractLeader extends AbstractRaftActorBehavior { return toRemove; } + @Override protected ClientRequestTracker findClientRequestTracker(long logIndex) { for (ClientRequestTracker tracker : trackerList) { if (tracker.getIndex() == logIndex) { @@ -330,8 +332,8 @@ public abstract class AbstractLeader extends AbstractRaftActorBehavior { mapFollowerToSnapshot.remove(followerId); if(LOG.isDebugEnabled()) { - LOG.debug("followerToLog.get(followerId).getNextIndex().get()=" + - followerToLog.get(followerId).getNextIndex().get()); + LOG.debug("followerToLog.get(followerId).getNextIndex()=" + + followerToLog.get(followerId).getNextIndex()); } if (mapFollowerToSnapshot.isEmpty()) { @@ -398,7 +400,7 @@ public abstract class AbstractLeader extends AbstractRaftActorBehavior { if (followerActor != null) { FollowerLogInformation followerLogInformation = followerToLog.get(followerId); - long followerNextIndex = followerLogInformation.getNextIndex().get(); + long followerNextIndex = followerLogInformation.getNextIndex(); boolean isFollowerActive = followerLogInformation.isFollowerActive(); List entries = null; @@ -484,7 +486,7 @@ public abstract class AbstractLeader extends AbstractRaftActorBehavior { final ActorSelection followerActor = context.getPeerActorSelection(e.getKey()); if (followerActor != null) { - long nextIndex = e.getValue().getNextIndex().get(); + long nextIndex = e.getValue().getNextIndex(); if (!context.getReplicatedLog().isPresent(nextIndex) && context.getReplicatedLog().isInSnapshot(nextIndex)) { @@ -535,7 +537,7 @@ public abstract class AbstractLeader extends AbstractRaftActorBehavior { ActorSelection followerActor = context.getPeerActorSelection(e.getKey()); if (followerActor != null) { - long nextIndex = e.getValue().getNextIndex().get(); + long nextIndex = e.getValue().getNextIndex(); if (!context.getReplicatedLog().isPresent(nextIndex) && context.getReplicatedLog().isInSnapshot(nextIndex)) { diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/LeaderTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/LeaderTest.java index 166fddd7b3..151015e97e 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/LeaderTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/LeaderTest.java @@ -72,6 +72,7 @@ public class LeaderTest extends AbstractRaftActorBehaviorTest { new JavaTestKit(getSystem()) {{ new Within(duration("1 seconds")) { + @Override protected void run() { ActorRef followerActor = getTestActor(); @@ -91,6 +92,7 @@ public class LeaderTest extends AbstractRaftActorBehaviorTest { final String out = new ExpectMsg(duration("1 seconds"), "match hint") { // do not put code outside this method, will run afterwards + @Override protected String match(Object in) { Object msg = fromSerializableMessage(in); if (msg instanceof AppendEntries) { @@ -116,6 +118,7 @@ public class LeaderTest extends AbstractRaftActorBehaviorTest { new JavaTestKit(getSystem()) {{ new Within(duration("1 seconds")) { + @Override protected void run() { ActorRef followerActor = getTestActor(); @@ -144,6 +147,7 @@ public class LeaderTest extends AbstractRaftActorBehaviorTest { final String out = new ExpectMsg(duration("1 seconds"), "match hint") { // do not put code outside this method, will run afterwards + @Override protected String match(Object in) { Object msg = fromSerializableMessage(in); if (msg instanceof AppendEntries) { @@ -168,6 +172,7 @@ public class LeaderTest extends AbstractRaftActorBehaviorTest { new JavaTestKit(getSystem()) {{ new Within(duration("1 seconds")) { + @Override protected void run() { ActorRef raftActor = getTestActor(); @@ -194,6 +199,7 @@ public class LeaderTest extends AbstractRaftActorBehaviorTest { new ExpectMsg(duration("1 seconds"), "match hint") { // do not put code outside this method, will run afterwards + @Override protected String match(Object in) { if (in instanceof ApplyState) { if (((ApplyState) in).getIdentifier().equals("state-id")) { @@ -481,6 +487,7 @@ public class LeaderTest extends AbstractRaftActorBehaviorTest { final String out = new ExpectMsg(duration("1 seconds"), "match hint") { // do not put code outside this method, will run afterwards + @Override protected String match(Object in) { if (in instanceof InstallSnapshotMessages.InstallSnapshot) { InstallSnapshot is = (InstallSnapshot) @@ -565,9 +572,9 @@ public class LeaderTest extends AbstractRaftActorBehaviorTest { assertEquals(1, leader.followerLogSize()); assertNotNull(leader.getFollower(followerActor.path().toString())); FollowerLogInformation fli = leader.getFollower(followerActor.path().toString()); - assertEquals(snapshotIndex, fli.getMatchIndex().get()); - assertEquals(snapshotIndex, fli.getMatchIndex().get()); - assertEquals(snapshotIndex + 1, fli.getNextIndex().get()); + assertEquals(snapshotIndex, fli.getMatchIndex()); + assertEquals(snapshotIndex, fli.getMatchIndex()); + assertEquals(snapshotIndex + 1, fli.getNextIndex()); }}; } @@ -778,6 +785,7 @@ public class LeaderTest extends AbstractRaftActorBehaviorTest { return createActorContext(leaderActor); } + @Override protected RaftActorContext createActorContext(ActorRef actorRef) { return new MockRaftActorContext("test", getSystem(), actorRef); } -- 2.36.6