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 <rovarga@cisco.com>

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 8b6871174180d8d372bcc2eb075ebef045ba6c3d..d33f780a2d82fb61afdcb9c5261bd8ffb3d5c6fb 100644 (file)
--- 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 {
  * @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;
 
     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());
             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);
             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 514346f1ab30334417b3f40c360759db1d5a7a3a..ec5f0244ecd0f47d323fba2dac10fc07bb63d3de 100644 (file)
--- 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);
     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;
 
     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);
 
 
         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());
 
 
         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.
         ((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);
 
 
         leaderActor.tell(new InitiateCaptureSnapshot(), leaderActor);
 


@@ -512,13 +513,13 @@ public class RaftActorServerConfigurationSupportTest extends AbstractActorTest {
         TestActorRef<MessageCollectorActor> leaderCollectorActor = newLeaderCollectorActor(leaderRaftActor);
 
         // Drop the commit message so the snapshot doesn't complete yet.
         TestActorRef<MessageCollectorActor> 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());
 
 
         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);
 
         // 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 a47e437e32cf3f694b7497ebec35761f35425cbb..a647554806034f5b21d52e04dd8530dd6bb4d51d 100644 (file)
--- 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;
  * </p>
  */
 public class Shard extends RaftActor {
  * </p>
  */
 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
 
     @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";
 
     // 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 5118de46c119a22bafa98258bd9afc8a052437fc..2da7e5ec238a0808ef4f6fd1169cf2dbc5aa6f69 100644 (file)
--- 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 {
  * @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;
 
     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 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;
             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 407cc609f6e2a263f8de61fa4ba8dae149f8c14a..d743e7903c9b25c40a2555bdca7574f2b0c6383a 100644 (file)
--- 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);
                         @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();
                                 }
                                 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);
 
                 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();
                     }
                 }
                         latch.get().countDown();
                     }
                 }