BUG-5280: eliminate ShardTransactionIdentifier 15/39015/25
authorRobert Varga <rovarga@cisco.com>
Tue, 17 May 2016 21:21:58 +0000 (23:21 +0200)
committerRobert Varga <rovarga@cisco.com>
Mon, 6 Jun 2016 13:43:44 +0000 (15:43 +0200)
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>
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/Shard.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardReadTransaction.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardReadWriteTransaction.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardSnapshotCohort.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardTransaction.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardTransactionFactory.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardWriteTransaction.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/identifiers/ShardTransactionIdentifier.java [deleted file]
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTransactionFailureTest.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardTransactionTest.java

index 250e3f8..b76c0fa 100644 (file)
@@ -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);
index e65b8f4..d28278a 100644 (file)
@@ -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;
     }
 
index 2cc23c9..46a5039 100644 (file)
@@ -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
index e49aa1e..7cdd7c6 100644 (file)
@@ -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);
     }
index c2a05a6..ee38d19 100644 (file)
@@ -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);
index 76838dd..3c03871 100644 (file)
@@ -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);
     }
 }
index 2ad2488..4ae54d3 100644 (file)
@@ -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
+++ /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;
-    }
-
-}
index 75fdc36..cc15e1a 100644 (file)
@@ -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");
index 95b2681..ad5a7a4 100644 (file)
@@ -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),