BUG-5280: eliminate ShardTransactionIdentifier

This is very simple wrapper class which has two functions:
- carries a String transaction identifier (making it available)
- prepends it with 'shard-' in its toString() method

Analyzing the actual uses of this class revealed that the toString()
is called only when instantiating an actor, at which point either
the transaction identifier is already present, or a transaction
identifier is actually available through AbstractShardDataTreeTransaction.

The second case is actually not entirely accurate, as for the case
of remotely-created transactions we will actually use the actor name
for the transaction name, hence adding to the overall string-induced
confusion.

Moving the toString() method towards actor instantiation removes
the need to pass this class around and we can pass down normal
transaction identifiers.

This newfound unification allows us to eliminate duplication
in the Shard*Transaction classes, which can pick the identifier
from AbstractShardDataTreeTransaction, eliminating surface where
inconsistencies may be introduced.

Change-Id: If05f1bf2e76434d07feab115239addbb4b4bfb91
Signed-off-by: Robert Varga <rovarga@cisco.com>

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 250e3f854f488049f33e6e87ba68baaf99797b46..b76c0fac34c2b9f7299eb69f230fc2c714bb235e 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
@@ -29,7 +29,6 @@ import org.opendaylight.controller.cluster.common.actor.MessageTracker.Error;
 import org.opendaylight.controller.cluster.common.actor.MeteringBehavior;
 import org.opendaylight.controller.cluster.datastore.exceptions.NoShardLeaderException;
 import org.opendaylight.controller.cluster.datastore.identifiers.ShardIdentifier;
-import org.opendaylight.controller.cluster.datastore.identifiers.ShardTransactionIdentifier;
 import org.opendaylight.controller.cluster.datastore.jmx.mbeans.shard.ShardMBeanFactory;
 import org.opendaylight.controller.cluster.datastore.jmx.mbeans.shard.ShardStats;
 import org.opendaylight.controller.cluster.datastore.messages.AbortTransaction;
@@ -602,8 +601,7 @@ public class Shard extends RaftActor {
         }
     }
 
-    private ActorRef createTransaction(int transactionType, String remoteTransactionId, String transactionChainId) {
-        ShardTransactionIdentifier transactionId = new ShardTransactionIdentifier(remoteTransactionId);
+    private ActorRef createTransaction(int transactionType, String transactionId, String transactionChainId) {
         LOG.debug("{}: Creating transaction : {} ", persistenceId(), transactionId);
         return transactionActorFactory.newShardTransaction(TransactionType.fromInt(transactionType),
             transactionId, transactionChainId);

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardReadTransaction.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardReadTransaction.java
index e65b8f415e587d2d25dcdc50f28b8edeea66abc4..d28278afa9dd5521f286fbf9fff50f7faf330762 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardReadTransaction.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardReadTransaction.java
@@ -28,8 +28,8 @@ public class ShardReadTransaction extends ShardTransaction {
     private final AbstractShardDataTreeTransaction<?> transaction;
 
     public ShardReadTransaction(AbstractShardDataTreeTransaction<?> transaction, ActorRef shardActor,
-            ShardStats shardStats, String transactionID) {
-        super(shardActor, shardStats, transactionID);
+            ShardStats shardStats) {
+        super(shardActor, shardStats, transaction.getId());
         this.transaction = transaction;
     }
 

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardReadWriteTransaction.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardReadWriteTransaction.java
index 2cc23c90e83399629bf2045259d5d25ec1c0a972..46a50390122057f3fce1cfe49c7c13343ff6f456 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardReadWriteTransaction.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardReadWriteTransaction.java
@@ -19,8 +19,8 @@ import org.opendaylight.controller.cluster.datastore.messages.ReadData;
  */
 public class ShardReadWriteTransaction extends ShardWriteTransaction {
     public ShardReadWriteTransaction(ReadWriteShardDataTreeTransaction transaction, ActorRef shardActor,
-            ShardStats shardStats, String transactionID) {
-        super(transaction, shardActor, shardStats, transactionID);
+            ShardStats shardStats) {
+        super(transaction, shardActor, shardStats);
     }
 
     @Override

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardSnapshotCohort.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardSnapshotCohort.java
index e49aa1e4f37d484d25439fd009affdeeba3a3cd8..7cdd7c6b98681cdc56f06f91552e6d0b191b9a8e 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardSnapshotCohort.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardSnapshotCohort.java
@@ -10,7 +10,6 @@ package org.opendaylight.controller.cluster.datastore;
 import akka.actor.ActorRef;
 import com.google.common.base.Preconditions;
 import java.util.concurrent.ExecutionException;
-import org.opendaylight.controller.cluster.datastore.identifiers.ShardTransactionIdentifier;
 import org.opendaylight.controller.cluster.datastore.messages.CreateSnapshot;
 import org.opendaylight.controller.cluster.datastore.utils.SerializationUtils;
 import org.opendaylight.controller.cluster.raft.RaftActorSnapshotCohort;
@@ -45,11 +44,8 @@ class ShardSnapshotCohort implements RaftActorSnapshotCohort {
         // so that this actor does not get block building the snapshot. THe transaction actor will
         // after processing the CreateSnapshot message.
 
-        ShardTransactionIdentifier transactionID = new ShardTransactionIdentifier(
-                "createSnapshot" + ++createSnapshotTransactionCounter);
-
         ActorRef createSnapshotTransaction = transactionActorFactory.newShardTransaction(
-                TransactionType.READ_ONLY, transactionID, "");
+                TransactionType.READ_ONLY, "createSnapshot" + ++createSnapshotTransactionCounter, "");
 
         createSnapshotTransaction.tell(CreateSnapshot.INSTANCE, actorRef);
     }

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardTransaction.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardTransaction.java
index c2a05a6d6130a3d3b28dd9879eebef70dcaf9e65..ee38d1918e13442dc6983a40cabc6ac64e77b65e 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardTransaction.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardTransaction.java
@@ -52,9 +52,8 @@ public abstract class ShardTransaction extends AbstractUntypedActorWithMetering
     }
 
     public static Props props(TransactionType type, AbstractShardDataTreeTransaction<?> transaction, ActorRef shardActor,
-            DatastoreContext datastoreContext, ShardStats shardStats, String transactionID) {
-        return Props.create(new ShardTransactionCreator(type, transaction, shardActor,
-           datastoreContext, shardStats, transactionID));
+            DatastoreContext datastoreContext, ShardStats shardStats) {
+        return Props.create(new ShardTransactionCreator(type, transaction, shardActor, datastoreContext, shardStats));
     }
 
     protected abstract AbstractShardDataTreeTransaction<?> getDOMStoreTransaction();
@@ -131,16 +130,14 @@ public abstract class ShardTransaction extends AbstractUntypedActorWithMetering
         final ActorRef shardActor;
         final DatastoreContext datastoreContext;
         final ShardStats shardStats;
-        final String transactionID;
         final TransactionType type;
 
         ShardTransactionCreator(TransactionType type, AbstractShardDataTreeTransaction<?> transaction, ActorRef shardActor,
-                DatastoreContext datastoreContext, ShardStats shardStats, String transactionID) {
+                DatastoreContext datastoreContext, ShardStats shardStats) {
             this.transaction = Preconditions.checkNotNull(transaction);
             this.shardActor = shardActor;
             this.shardStats = shardStats;
             this.datastoreContext = datastoreContext;
-            this.transactionID = Preconditions.checkNotNull(transactionID);
             this.type = type;
         }
 
@@ -149,16 +146,13 @@ public abstract class ShardTransaction extends AbstractUntypedActorWithMetering
             final ShardTransaction tx;
             switch (type) {
             case READ_ONLY:
-                tx = new ShardReadTransaction(transaction, shardActor,
-                    shardStats, transactionID);
+                tx = new ShardReadTransaction(transaction, shardActor, shardStats);
                 break;
             case READ_WRITE:
-                tx = new ShardReadWriteTransaction((ReadWriteShardDataTreeTransaction)transaction,
-                    shardActor, shardStats, transactionID);
+                tx = new ShardReadWriteTransaction((ReadWriteShardDataTreeTransaction)transaction, shardActor, shardStats);
                 break;
             case WRITE_ONLY:
-                tx = new ShardWriteTransaction((ReadWriteShardDataTreeTransaction)transaction,
-                    shardActor, shardStats, transactionID);
+                tx = new ShardWriteTransaction((ReadWriteShardDataTreeTransaction)transaction, shardActor, shardStats);
                 break;
             default:
                 throw new IllegalArgumentException("Unhandled transaction type " + type);

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardTransactionFactory.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardTransactionFactory.java
index 76838dd2f83dc4bec45730e741dc5b8b47ad1686..3c0387115900d54c17eafb8512736eeef2933542 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardTransactionFactory.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardTransactionFactory.java
@@ -10,7 +10,6 @@ package org.opendaylight.controller.cluster.datastore;
 import akka.actor.ActorRef;
 import akka.actor.UntypedActorContext;
 import com.google.common.base.Preconditions;
-import org.opendaylight.controller.cluster.datastore.identifiers.ShardTransactionIdentifier;
 import org.opendaylight.controller.cluster.datastore.jmx.mbeans.shard.ShardStats;
 
 /**
@@ -37,28 +36,26 @@ class ShardTransactionActorFactory {
         this.shardActor = shardActor;
     }
 
-    ActorRef newShardTransaction(TransactionType type, ShardTransactionIdentifier transactionID,
-            String transactionChainID) {
+    ActorRef newShardTransaction(TransactionType type, String transactionID, String transactionChainID) {
         final AbstractShardDataTreeTransaction<?> transaction;
         switch (type) {
         case READ_ONLY:
-            transaction = dataTree.newReadOnlyTransaction(transactionID.toString(), transactionChainID);
+            transaction = dataTree.newReadOnlyTransaction(transactionID, transactionChainID);
             shardMBean.incrementReadOnlyTransactionCount();
             break;
         case READ_WRITE:
-            transaction = dataTree.newReadWriteTransaction(transactionID.toString(), transactionChainID);
+            transaction = dataTree.newReadWriteTransaction(transactionID, transactionChainID);
             shardMBean.incrementReadWriteTransactionCount();
             break;
         case WRITE_ONLY:
-            transaction = dataTree.newReadWriteTransaction(transactionID.toString(), transactionChainID);
+            transaction = dataTree.newReadWriteTransaction(transactionID, transactionChainID);
             shardMBean.incrementWriteOnlyTransactionCount();
             break;
         default:
             throw new IllegalArgumentException("Unsupported transaction type " + type);
         }
 
-        return actorContext.actorOf(ShardTransaction.props(type, transaction, shardActor, datastoreContext, shardMBean,
-                transactionID.getRemoteTransactionId()).withDispatcher(txnDispatcherPath),
-                transactionID.toString());
+        return actorContext.actorOf(ShardTransaction.props(type, transaction, shardActor, datastoreContext, shardMBean)
+            .withDispatcher(txnDispatcherPath), "shard-" + transactionID);
     }
 }

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardWriteTransaction.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardWriteTransaction.java
index 2ad2488d6d652263c07327983c97e6b429a9c625..4ae54d3e5f727856c93a6f896e7630f21e11c10d 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardWriteTransaction.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardWriteTransaction.java
@@ -30,8 +30,8 @@ public class ShardWriteTransaction extends ShardTransaction {
     private final ReadWriteShardDataTreeTransaction transaction;
 
     public ShardWriteTransaction(ReadWriteShardDataTreeTransaction transaction, ActorRef shardActor,
-            ShardStats shardStats, String transactionID) {
-        super(shardActor, shardStats, transactionID);
+            ShardStats shardStats) {
+        super(shardActor, shardStats, transaction.getId());
         this.transaction = transaction;
     }
 

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/identifiers/ShardTransactionIdentifier.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/identifiers/ShardTransactionIdentifier.java
deleted file mode 100644 (file)
index fa1525c..0000000
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/identifiers/ShardTransactionIdentifier.java
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * Copyright (c) 2014 Cisco Systems, Inc. and others.  All rights reserved.
- *
- * This program and the accompanying materials are made available under the
- * 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.datastore.identifiers;
-
-import com.google.common.base.Preconditions;
-
-public class ShardTransactionIdentifier {
-    private final String remoteTransactionId;
-    private final String stringRepresentation;
-
-    public ShardTransactionIdentifier(String remoteTransactionId) {
-        this.remoteTransactionId = Preconditions.checkNotNull(remoteTransactionId,
-                "remoteTransactionId should not be null");
-
-        stringRepresentation = new StringBuilder(remoteTransactionId.length() + 6).append("shard-").
-                append(remoteTransactionId).toString();
-    }
-
-    public String getRemoteTransactionId() {
-        return remoteTransactionId;
-    }
-
-    @Override
-    public boolean equals(Object o) {
-        if (this == o) {
-            return true;
-        }
-        if (o == null || getClass() != o.getClass()) {
-            return false;
-        }
-
-        ShardTransactionIdentifier that = (ShardTransactionIdentifier) o;
-
-        if (!remoteTransactionId.equals(that.remoteTransactionId)) {
-            return false;
-        }
-
-        return true;
-    }
-
-    @Override
-    public int hashCode() {
-        return remoteTransactionId.hashCode();
-    }
-
-    @Override public String toString() {
-        return stringRepresentation;
-    }
-
-}

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTransactionFailureTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTransactionFailureTest.java
index 75fdc361461ba2836281d8c01e9c181fa9418db8..cc15e1a0e2b1e439324b8856b2079da22a69580a 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTransactionFailureTest.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTransactionFailureTest.java
@@ -60,7 +60,7 @@ public class ShardTransactionFailureTest extends AbstractActorTest {
 
         final ActorRef shard = createShard();
         final Props props = ShardTransaction.props(RO, store.newReadOnlyTransaction("test-txn", null), shard,
-                datastoreContext, shardStats, "txn");
+                datastoreContext, shardStats);
 
         final TestActorRef<ShardTransaction> subject = TestActorRef.create(getSystem(), props,
                 "testNegativeReadWithReadOnlyTransactionClosed");
@@ -82,7 +82,7 @@ public class ShardTransactionFailureTest extends AbstractActorTest {
 
         final ActorRef shard = createShard();
         final Props props = ShardTransaction.props(RW, store.newReadWriteTransaction("test-txn", null), shard,
-                datastoreContext, shardStats, "txn");
+                datastoreContext, shardStats);
 
         final TestActorRef<ShardTransaction> subject = TestActorRef.create(getSystem(), props,
                 "testNegativeReadWithReadWriteTransactionClosed");
@@ -103,7 +103,7 @@ public class ShardTransactionFailureTest extends AbstractActorTest {
 
         final ActorRef shard = createShard();
         final Props props = ShardTransaction.props(RW, store.newReadWriteTransaction("test-txn", null), shard,
-                datastoreContext, shardStats, "txn");
+                datastoreContext, shardStats);
 
         final TestActorRef<ShardTransaction> subject = TestActorRef.create(getSystem(), props,
                 "testNegativeExistsWithReadWriteTransactionClosed");

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTransactionTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTransactionTest.java
index 95b26817b970642f50323ce99e6d236adde1c347..ad5a7a45350e1ca1d329d99dbe47295d98a399bf 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTransactionTest.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTransactionTest.java
@@ -86,7 +86,7 @@ public class ShardTransactionTest extends AbstractActorTest {
 
     private ActorRef newTransactionActor(TransactionType type, AbstractShardDataTreeTransaction<?> transaction, ActorRef shard, String name) {
         Props props = ShardTransaction.props(type, transaction, shard != null ? shard : createShard(),
-                datastoreContext, shardStats, "txn");
+                datastoreContext, shardStats);
         return getSystem().actorOf(props, name);
     }
 
@@ -420,7 +420,7 @@ public class ShardTransactionTest extends AbstractActorTest {
     public void testNegativePerformingWriteOperationOnReadTransaction() throws Exception {
         final ActorRef shard = createShard();
         final Props props = ShardTransaction.props(TransactionType.READ_ONLY, readOnlyTransaction(), shard,
-                datastoreContext, shardStats, "txn");
+                datastoreContext, shardStats);
         final TestActorRef<ShardTransaction> transaction = TestActorRef.apply(props,getSystem());
 
         transaction.receive(new BatchedModifications("tx1", DataStoreVersions.CURRENT_VERSION, null),