From d806210832a39d23be77d31d8528775928ca7504 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Thu, 15 Jun 2017 23:13:06 -0400 Subject: [PATCH] Enforce non-null entries field in AppendEntries The List entries field has to be non-null so enforce it with PreCondition. Same with leaderId. Callers of getEntries do not need to check for null. Change-Id: I5cd404d54e453e43456952ea2e11ea7f8f1c626c Signed-off-by: Tcm Pantelis --- .../cluster/raft/behaviors/Follower.java | 18 ++++++++---------- .../cluster/raft/messages/AppendEntries.java | 13 +++++++++---- .../AbstractRaftActorBehaviorTest.java | 6 ++++-- .../cluster/raft/behaviors/FollowerTest.java | 3 ++- 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java index 8aee8c1af8..1587719417 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java @@ -126,7 +126,7 @@ public class Follower extends AbstractRaftActorBehavior { @Override protected RaftActorBehavior handleAppendEntries(final ActorRef sender, final AppendEntries appendEntries) { - int numLogEntries = appendEntries.getEntries() != null ? appendEntries.getEntries().size() : 0; + int numLogEntries = appendEntries.getEntries().size(); if (log.isTraceEnabled()) { log.trace("{}: handleAppendEntries: {}", logName(), appendEntries); } else if (log.isDebugEnabled() && numLogEntries > 0) { @@ -174,10 +174,8 @@ public class Follower extends AbstractRaftActorBehavior { return this; } - if (appendEntries.getEntries() != null && appendEntries.getEntries().size() > 0) { - - log.debug("{}: Number of entries to be appended = {}", logName(), - appendEntries.getEntries().size()); + if (numLogEntries > 0) { + log.debug("{}: Number of entries to be appended = {}", logName(), numLogEntries); // 3. If an existing entry conflicts with a new one (same index // but different terms), delete the existing entry and all that @@ -186,7 +184,7 @@ public class Follower extends AbstractRaftActorBehavior { if (context.getReplicatedLog().size() > 0) { // Find the entry up until the one that is not in the follower's log - for (int i = 0;i < appendEntries.getEntries().size(); i++, addEntriesFrom++) { + for (int i = 0;i < numLogEntries; i++, addEntriesFrom++) { ReplicatedLogEntry matchEntry = appendEntries.getEntries().get(i); if (!isLogEntryPresent(matchEntry.getIndex())) { @@ -246,15 +244,15 @@ public class Follower extends AbstractRaftActorBehavior { // purged from the persisted log as well. final AtomicBoolean shouldCaptureSnapshot = new AtomicBoolean(false); final Procedure appendAndPersistCallback = logEntry -> { - final ReplicatedLogEntry lastEntryToAppend = appendEntries.getEntries().get( - appendEntries.getEntries().size() - 1); + final List entries = appendEntries.getEntries(); + final ReplicatedLogEntry lastEntryToAppend = entries.get(entries.size() - 1); if (shouldCaptureSnapshot.get() && logEntry == lastEntryToAppend) { context.getSnapshotManager().capture(context.getReplicatedLog().last(), getReplicatedToAllIndex()); } }; // 4. Append any new entries not already in the log - for (int i = addEntriesFrom; i < appendEntries.getEntries().size(); i++) { + for (int i = addEntriesFrom; i < numLogEntries; i++) { ReplicatedLogEntry entry = appendEntries.getEntries().get(i); log.debug("{}: Append entry to log {}", logName(), entry.getData()); @@ -370,7 +368,7 @@ public class Follower extends AbstractRaftActorBehavior { } final List entries = appendEntries.getEntries(); - if (entries != null && entries.size() > 0 && !isLogEntryPresent(entries.get(0).getIndex() - 1)) { + if (entries.size() > 0 && !isLogEntryPresent(entries.get(0).getIndex() - 1)) { log.info("{}: Cannot append entries because the calculated previousIndex {} was not found in the " + "in-memory journal", logName(), entries.get(0).getIndex() - 1); return true; diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/AppendEntries.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/AppendEntries.java index 017794679c..d878e2c9c3 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/AppendEntries.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/AppendEntries.java @@ -8,12 +8,14 @@ package org.opendaylight.controller.cluster.raft.messages; +import com.google.common.base.Preconditions; import java.io.Externalizable; import java.io.IOException; import java.io.ObjectInput; import java.io.ObjectOutput; import java.util.ArrayList; import java.util.List; +import javax.annotation.Nonnull; import org.opendaylight.controller.cluster.raft.ReplicatedLogEntry; import org.opendaylight.controller.cluster.raft.persisted.SimpleReplicatedLogEntry; import org.opendaylight.controller.cluster.raft.protobuff.client.messages.Payload; @@ -45,18 +47,20 @@ public class AppendEntries extends AbstractRaftRPC { private final short payloadVersion; - public AppendEntries(long term, String leaderId, long prevLogIndex, long prevLogTerm, - List entries, long leaderCommit, long replicatedToAllIndex, short payloadVersion) { + public AppendEntries(long term, @Nonnull String leaderId, long prevLogIndex, long prevLogTerm, + @Nonnull List entries, long leaderCommit, long replicatedToAllIndex, + short payloadVersion) { super(term); - this.leaderId = leaderId; + this.leaderId = Preconditions.checkNotNull(leaderId); this.prevLogIndex = prevLogIndex; this.prevLogTerm = prevLogTerm; - this.entries = entries; + this.entries = Preconditions.checkNotNull(entries); this.leaderCommit = leaderCommit; this.replicatedToAllIndex = replicatedToAllIndex; this.payloadVersion = payloadVersion; } + @Nonnull public String getLeaderId() { return leaderId; } @@ -69,6 +73,7 @@ public class AppendEntries extends AbstractRaftRPC { return prevLogTerm; } + @Nonnull public List getEntries() { return entries; } diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractRaftActorBehaviorTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractRaftActorBehaviorTest.java index 3dba8d240f..03950dbd4b 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractRaftActorBehaviorTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractRaftActorBehaviorTest.java @@ -20,6 +20,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.ObjectOutputStream; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; import org.junit.After; @@ -101,7 +102,8 @@ public abstract class AbstractRaftActorBehaviorTest // First set the receivers term to a high number (1000) context.getTermInformation().update(1000, "test"); - AppendEntries appendEntries = new AppendEntries(100, "leader-1", 0, 0, null, 101, -1, (short)4); + AppendEntries appendEntries = new AppendEntries(100, "leader-1", 0, 0, Collections.emptyList(), 101, -1, + (short)4); behavior = createBehavior(context); @@ -329,7 +331,7 @@ public abstract class AbstractRaftActorBehaviorTest } protected AppendEntries createAppendEntriesWithNewerTerm() { - return new AppendEntries(100, "leader-1", 0, 0, null, 1, -1, (short)0); + return new AppendEntries(100, "leader-1", 0, 0, Collections.emptyList(), 1, -1, (short)0); } protected AppendEntriesReply createAppendEntriesReplyWithNewerTerm() { diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/FollowerTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/FollowerTest.java index 15fca0cb8d..37a8b6b906 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/FollowerTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/FollowerTest.java @@ -538,7 +538,8 @@ public class FollowerTest extends AbstractRaftActorBehaviorTest { // AppendEntries is now sent with a bigger term // this will set the receivers term to be the same as the sender's term - AppendEntries appendEntries = new AppendEntries(100, "leader", 0, 0, null, 101, -1, (short)0); + AppendEntries appendEntries = new AppendEntries(100, "leader", 0, 0, Collections.emptyList(), 101, -1, + (short)0); follower = createBehavior(context); -- 2.36.6