From 6313c088fc7db266cc25b691e0cd909300fc8425 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Fri, 1 Apr 2016 16:14:25 +0200 Subject: [PATCH] Do not use Strings for internal messages Using a String for the message identity has the effect of the equality running on Strings, which has the effect that message is not really isolated, but can be sent by anyone who knows the message content. Instantiate simple Objects instead, which force a proper identity equality check. Retain debuggability by having these instances override toString(). Change-Id: Ia581e0a1e023ace10b5dbb81a44092ca04a4ff8f Signed-off-by: Robert Varga --- .../raft/RaftActorSnapshotMessageSupport.java | 9 +++++++-- .../RaftActorServerConfigurationSupportTest.java | 9 +++++---- .../controller/cluster/datastore/Shard.java | 16 +++++++++++++--- .../EntityOwnershipShardCommitCoordinator.java | 9 +++++++-- .../controller/cluster/datastore/ShardTest.java | 5 +++-- 5 files changed, 35 insertions(+), 13 deletions(-) diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java index 8b68711741..d33f780a2d 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java @@ -29,7 +29,12 @@ import scala.concurrent.duration.Duration; * @author Thomas Pantelis */ class RaftActorSnapshotMessageSupport { - static final String COMMIT_SNAPSHOT = "commit_snapshot"; + static final Object COMMIT_SNAPSHOT = new Object() { + @Override + public String toString() { + return "commit_snapshot"; + } + }; private final RaftActorContext context; private final RaftActorSnapshotCohort cohort; @@ -69,7 +74,7 @@ class RaftActorSnapshotMessageSupport { onSaveSnapshotFailure((SaveSnapshotFailure) message); } else if (message instanceof CaptureSnapshotReply) { onCaptureSnapshotReply(((CaptureSnapshotReply) message).getSnapshot()); - } else if (message.equals(COMMIT_SNAPSHOT)) { + } else if (COMMIT_SNAPSHOT.equals(message)) { context.getSnapshotManager().commit(-1); } else if (message instanceof GetSnapshot) { onGetSnapshot(sender); diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorServerConfigurationSupportTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorServerConfigurationSupportTest.java index 514346f1ab..ec5f0244ec 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorServerConfigurationSupportTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorServerConfigurationSupportTest.java @@ -80,6 +80,7 @@ public class RaftActorServerConfigurationSupportTest extends AbstractActorTest { static final String NEW_SERVER_ID = "new-server"; static final String NEW_SERVER_ID2 = "new-server2"; private static final Logger LOG = LoggerFactory.getLogger(RaftActorServerConfigurationSupportTest.class); + private static final Class COMMIT_MESSAGE_CLASS = RaftActorSnapshotMessageSupport.COMMIT_SNAPSHOT.getClass(); private static final boolean NO_PERSISTENCE = false; private static final boolean PERSISTENT = true; @@ -436,7 +437,7 @@ public class RaftActorServerConfigurationSupportTest extends AbstractActorTest { leaderActor.tell(new InitiateCaptureSnapshot(), leaderActor); - String commitMsg = expectFirstMatching(leaderCollectorActor, String.class); + Object commitMsg = expectFirstMatching(leaderCollectorActor, COMMIT_MESSAGE_CLASS); leaderActor.tell(new AddServer(NEW_SERVER_ID, newFollowerRaftActor.path().toString(), true), testKit.getRef()); @@ -479,7 +480,7 @@ public class RaftActorServerConfigurationSupportTest extends AbstractActorTest { ((DefaultConfigParamsImpl)leaderActorContext.getConfigParams()).setElectionTimeoutFactor(1); // Drop commit message so the snapshot doesn't complete. - leaderRaftActor.setDropMessageOfType(String.class); + leaderRaftActor.setDropMessageOfType(COMMIT_MESSAGE_CLASS); leaderActor.tell(new InitiateCaptureSnapshot(), leaderActor); @@ -512,13 +513,13 @@ public class RaftActorServerConfigurationSupportTest extends AbstractActorTest { TestActorRef leaderCollectorActor = newLeaderCollectorActor(leaderRaftActor); // Drop the commit message so the snapshot doesn't complete yet. - leaderRaftActor.setDropMessageOfType(String.class); + leaderRaftActor.setDropMessageOfType(COMMIT_MESSAGE_CLASS); leaderActor.tell(new InitiateCaptureSnapshot(), leaderActor); leaderActor.tell(new AddServer(NEW_SERVER_ID, newFollowerRaftActor.path().toString(), true), testKit.getRef()); - String commitMsg = expectFirstMatching(leaderCollectorActor, String.class); + Object commitMsg = expectFirstMatching(leaderCollectorActor, COMMIT_MESSAGE_CLASS); // Change the leader behavior to follower leaderActor.tell(new Follower(leaderActorContext), leaderActor); diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/Shard.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/Shard.java index a47e437e32..a647554806 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/Shard.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/Shard.java @@ -82,11 +82,21 @@ import scala.concurrent.duration.FiniteDuration; *

*/ public class Shard extends RaftActor { - - protected static final Object TX_COMMIT_TIMEOUT_CHECK_MESSAGE = "txCommitTimeoutCheck"; + @VisibleForTesting + static final Object TX_COMMIT_TIMEOUT_CHECK_MESSAGE = new Object() { + @Override + public String toString() { + return "txCommitTimeoutCheck"; + } + }; @VisibleForTesting - static final Object GET_SHARD_MBEAN_MESSAGE = "getShardMBeanMessage"; + static final Object GET_SHARD_MBEAN_MESSAGE = new Object() { + @Override + public String toString() { + return "getShardMBeanMessage"; + } + }; // FIXME: shard names should be encapsulated in their own class and this should be exposed as a constant. public static final String DEFAULT_NAME = "default"; diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java index 5118de46c1..2da7e5ec23 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/entityownership/EntityOwnershipShardCommitCoordinator.java @@ -28,7 +28,12 @@ import scala.concurrent.duration.FiniteDuration; * @author Thomas Pantelis */ class EntityOwnershipShardCommitCoordinator { - private static final Object COMMIT_RETRY_MESSAGE = "entityCommitRetry"; + private static final Object COMMIT_RETRY_MESSAGE = new Object() { + @Override + public String toString() { + return "entityCommitRetry"; + } + }; private final Logger log; private int transactionIDCounter = 0; @@ -50,7 +55,7 @@ class EntityOwnershipShardCommitCoordinator { } else if(message instanceof akka.actor.Status.Failure) { // Failure reply from a local commit. inflightCommitFailure(((Failure)message).cause(), shard); - } else if(message.equals(COMMIT_RETRY_MESSAGE)) { + } else if(COMMIT_RETRY_MESSAGE.equals(message)) { retryInflightCommit(shard); } else { handled = false; diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTest.java index 407cc609f6..d743e7903c 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTest.java @@ -2013,7 +2013,7 @@ public class ShardTest extends AbstractShardTest { @Override public void handleCommand(final Object message) { super.handleCommand(message); - if(message.equals(TX_COMMIT_TIMEOUT_CHECK_MESSAGE)) { + if(TX_COMMIT_TIMEOUT_CHECK_MESSAGE.equals(message)) { if(cleaupCheckLatch.get() != null) { cleaupCheckLatch.get().countDown(); } @@ -2110,7 +2110,8 @@ public class ShardTest extends AbstractShardTest { public void handleCommand(final Object message) { super.handleCommand(message); - if (message instanceof SaveSnapshotSuccess || message.equals("commit_snapshot")) { + // XXX: commit_snapshot equality check references RaftActorSnapshotMessageSupport.COMMIT_SNAPSHOT + if (message instanceof SaveSnapshotSuccess || "commit_snapshot".equals(message.toString())) { latch.get().countDown(); } } -- 2.36.6