Bug 4774: Wait for prior RO tx creates on tx chain

Added a priorReadOnlyTxPromises map to TransactionChainProxy that holds
Promise instances for each read-only tx. When the parent class completes
the primary shard lookup and creates the TransactionContext (either success or
failure), onTransactionContextCreated is called which completes the Promise. A
write tx that is created prior to completion will wait on the Promise's Future via
findPrimaryShard.

Change-Id: Ib1a620cfd5be3e38f633b3faf9ef7a31abaaf345
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/AbstractTransactionContextFactory.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/AbstractTransactionContextFactory.java
index a0071c3f47d7ae0b9a147980eb22ec377d5660ba..4fda059f3182ee898f7f4bc076ae7800159b3a77 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/AbstractTransactionContextFactory.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/AbstractTransactionContextFactory.java
@@ -55,7 +55,12 @@ abstract class AbstractTransactionContextFactory<F extends LocalTransactionFacto
                 LOG.debug("Tx {} - Creating local component for shard {} using factory {}",
                         parent.getIdentifier(), shardName, local);
             }
                 LOG.debug("Tx {} - Creating local component for shard {} using factory {}",
                         parent.getIdentifier(), shardName, local);
             }

-            return createLocalTransactionContext(local, parent);
+
+            try {
+                return createLocalTransactionContext(local, parent);
+            } catch(Exception e) {
+                return new NoOpTransactionContext(e, parent.getIdentifier());
+            }

         }
 
         return null;
         }
 
         return null;


@@ -70,29 +75,37 @@ abstract class AbstractTransactionContextFactory<F extends LocalTransactionFacto
 
         updateShardInfo(shardName, primaryShardInfo);
 
 
         updateShardInfo(shardName, primaryShardInfo);
 

-        TransactionContext localContext = maybeCreateLocalTransactionContext(parent, shardName);
-        if(localContext != null) {
-            transactionContextWrapper.executePriorTransactionOperations(localContext);
-        } else {
-            RemoteTransactionContextSupport remote = new RemoteTransactionContextSupport(transactionContextWrapper,
-                    parent, shardName);
-            remote.setPrimaryShard(primaryShardInfo.getPrimaryShardActor(), primaryShardInfo.getPrimaryShardVersion());
+        try {
+            TransactionContext localContext = maybeCreateLocalTransactionContext(parent, shardName);
+            if(localContext != null) {
+                transactionContextWrapper.executePriorTransactionOperations(localContext);
+            } else {
+                RemoteTransactionContextSupport remote = new RemoteTransactionContextSupport(transactionContextWrapper,
+                        parent, shardName);
+                remote.setPrimaryShard(primaryShardInfo.getPrimaryShardActor(), primaryShardInfo.getPrimaryShardVersion());
+            }
+        } finally {
+            onTransactionContextCreated(parent.getIdentifier());

         }
     }
 
         }
     }
 

-    private static void onFindPrimaryShardFailure(Throwable failure, TransactionProxy parent,
+    private void onFindPrimaryShardFailure(Throwable failure, TransactionProxy parent,

             String shardName, TransactionContextWrapper transactionContextWrapper) {
         LOG.debug("Tx {}: Find primary for shard {} failed", parent.getIdentifier(), shardName, failure);
 
             String shardName, TransactionContextWrapper transactionContextWrapper) {
         LOG.debug("Tx {}: Find primary for shard {} failed", parent.getIdentifier(), shardName, failure);
 

-        transactionContextWrapper.executePriorTransactionOperations(new NoOpTransactionContext(failure,
-                parent.getIdentifier()));
+        try {
+            transactionContextWrapper.executePriorTransactionOperations(new NoOpTransactionContext(failure,
+                    parent.getIdentifier()));
+        } finally {
+            onTransactionContextCreated(parent.getIdentifier());
+        }

     }
 
     final TransactionContextWrapper newTransactionContextWrapper(final TransactionProxy parent, final String shardName) {
         final TransactionContextWrapper transactionContextWrapper =
                 new TransactionContextWrapper(parent.getIdentifier(), actorContext);
 
     }
 
     final TransactionContextWrapper newTransactionContextWrapper(final TransactionProxy parent, final String shardName) {
         final TransactionContextWrapper transactionContextWrapper =
                 new TransactionContextWrapper(parent.getIdentifier(), actorContext);
 

-        Future<PrimaryShardInfo> findPrimaryFuture = findPrimaryShard(shardName, parent.getIdentifier().toString());
+        Future<PrimaryShardInfo> findPrimaryFuture = findPrimaryShard(shardName, parent.getIdentifier());

         if(findPrimaryFuture.isCompleted()) {
             Try<PrimaryShardInfo> maybe = findPrimaryFuture.value().get();
             if(maybe.isSuccess()) {
         if(findPrimaryFuture.isCompleted()) {
             Try<PrimaryShardInfo> maybe = findPrimaryFuture.value().get();
             if(maybe.isSuccess()) {


@@ -154,7 +167,8 @@ abstract class AbstractTransactionContextFactory<F extends LocalTransactionFacto
      * @param shardName Shard name
      * @return Future containing shard information.
      */
      * @param shardName Shard name
      * @return Future containing shard information.
      */

-    protected abstract Future<PrimaryShardInfo> findPrimaryShard(String shardName, String txId);
+    protected abstract Future<PrimaryShardInfo> findPrimaryShard(@Nonnull String shardName,
+            @Nonnull TransactionIdentifier txId);

 
     /**
      * Create local transaction factory for specified shard, backed by specified shard leader
 
     /**
      * Create local transaction factory for specified shard, backed by specified shard leader


@@ -175,6 +189,13 @@ abstract class AbstractTransactionContextFactory<F extends LocalTransactionFacto
      */
     protected abstract <T> void onTransactionReady(@Nonnull TransactionIdentifier transaction, @Nonnull Collection<Future<T>> cohortFutures);
 
      */
     protected abstract <T> void onTransactionReady(@Nonnull TransactionIdentifier transaction, @Nonnull Collection<Future<T>> cohortFutures);
 

+    /**
+     * Callback invoked when the internal TransactionContext has been created for a transaction.
+     *
+     * @param transactionId the ID of the transaction.
+     */
+    protected abstract void onTransactionContextCreated(@Nonnull TransactionIdentifier transactionId);
+

     private static TransactionContext createLocalTransactionContext(final LocalTransactionFactory factory,
                                                                     final TransactionProxy parent) {
 
     private static TransactionContext createLocalTransactionContext(final LocalTransactionFactory factory,
                                                                     final TransactionProxy parent) {
 

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/TransactionChainProxy.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/TransactionChainProxy.java
index 4b75fbbd5cdbf61dde4c759896b62a521a3be048..d230a956c250d0bb359c2889bffa18ac4346d48f 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/TransactionChainProxy.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/TransactionChainProxy.java
@@ -8,11 +8,18 @@
 package org.opendaylight.controller.cluster.datastore;
 
 import akka.actor.ActorSelection;
 package org.opendaylight.controller.cluster.datastore;
 
 import akka.actor.ActorSelection;

+import akka.dispatch.Futures;

 import akka.dispatch.OnComplete;
 import com.google.common.base.Preconditions;
 import akka.dispatch.OnComplete;
 import com.google.common.base.Preconditions;

+import java.util.ArrayList;

 import java.util.Collection;
 import java.util.Collection;

+import java.util.List;
+import java.util.Map.Entry;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;

 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;

+import javax.annotation.Nonnull;

 import org.opendaylight.controller.cluster.datastore.identifiers.TransactionChainIdentifier;
 import org.opendaylight.controller.cluster.datastore.identifiers.TransactionIdentifier;
 import org.opendaylight.controller.cluster.datastore.messages.CloseTransactionChain;
 import org.opendaylight.controller.cluster.datastore.identifiers.TransactionChainIdentifier;
 import org.opendaylight.controller.cluster.datastore.identifiers.TransactionIdentifier;
 import org.opendaylight.controller.cluster.datastore.messages.CloseTransactionChain;


@@ -120,6 +127,27 @@ final class TransactionChainProxy extends AbstractTransactionContextFactory<Loca
     private final TransactionContextFactory parent;
     private volatile State currentState = IDLE_STATE;
 
     private final TransactionContextFactory parent;
     private volatile State currentState = IDLE_STATE;
 

+    /**
+     * This map holds Promise instances for each read-only tx. It is used to maintain ordering of tx creates
+     * wrt to read-only tx's between this class and a LocalTransactionChain since they're bridged by
+     * asynchronous futures. Otherwise, in the following scenario, eg:
+     *
+     *   1) Create write tx1 on chain
+     *   2) do write and submit
+     *   3) Create read-only tx2 on chain and issue read
+     *   4) Create write tx3 on chain, do write but do not submit
+     *
+     * if the sequence/timing is right, tx3 may create its local tx on the LocalTransactionChain before tx2,
+     * which results in tx2 failing b/c tx3 isn't ready yet. So maintaining ordering prevents this issue
+     * (see Bug 4774).
+     * <p>
+     * A Promise is added via newReadOnlyTransaction. When the parent class completes the primary shard
+     * lookup and creates the TransactionContext (either success or failure), onTransactionContextCreated is
+     * called which completes the Promise. A write tx that is created prior to completion will wait on the
+     * Promise's Future via findPrimaryShard.
+     */
+    private final ConcurrentMap<TransactionIdentifier, Promise<Object>> priorReadOnlyTxPromises = new ConcurrentHashMap<>();
+

     TransactionChainProxy(final TransactionContextFactory parent) {
         super(parent.getActorContext());
 
     TransactionChainProxy(final TransactionContextFactory parent) {
         super(parent.getActorContext());
 


@@ -134,7 +162,9 @@ final class TransactionChainProxy extends AbstractTransactionContextFactory<Loca
     @Override
     public DOMStoreReadTransaction newReadOnlyTransaction() {
         currentState.checkReady();
     @Override
     public DOMStoreReadTransaction newReadOnlyTransaction() {
         currentState.checkReady();

-        return new TransactionProxy(this, TransactionType.READ_ONLY);
+        TransactionProxy transactionProxy = new TransactionProxy(this, TransactionType.READ_ONLY);
+        priorReadOnlyTxPromises.put(transactionProxy.getIdentifier(), Futures.<Object>promise());
+        return transactionProxy;

     }
 
     @Override
     }
 
     @Override


@@ -178,15 +208,16 @@ final class TransactionChainProxy extends AbstractTransactionContextFactory<Loca
      * before we initiate the next Tx in the chain to avoid creation failures if the
      * previous Tx's ready operations haven't completed yet.
      */
      * before we initiate the next Tx in the chain to avoid creation failures if the
      * previous Tx's ready operations haven't completed yet.
      */

+    @SuppressWarnings({ "unchecked", "rawtypes" })

     @Override
     @Override

-    protected Future<PrimaryShardInfo> findPrimaryShard(final String shardName, final String txId) {
+    protected Future<PrimaryShardInfo> findPrimaryShard(final String shardName, final TransactionIdentifier txId) {

         // Read current state atomically
         final State localState = currentState;
 
         // There are no outstanding futures, shortcut
         // Read current state atomically
         final State localState = currentState;
 
         // There are no outstanding futures, shortcut

-        final Future<?> previous = localState.previousFuture();
+        Future<?> previous = localState.previousFuture();

         if (previous == null) {
         if (previous == null) {

-            return parent.findPrimaryShard(shardName, txId);
+            return combineFutureWithPossiblePriorReadOnlyTxFutures(parent.findPrimaryShard(shardName, txId), txId);

         }
 
         final String previousTransactionId;
         }
 
         final String previousTransactionId;


@@ -199,8 +230,10 @@ final class TransactionChainProxy extends AbstractTransactionContextFactory<Loca
             LOG.debug("Waiting for ready futures on chain {}", getTransactionChainId());
         }
 
             LOG.debug("Waiting for ready futures on chain {}", getTransactionChainId());
         }
 

+        previous = combineFutureWithPossiblePriorReadOnlyTxFutures(previous, txId);
+

         // Add a callback for completion of the combined Futures.
         // Add a callback for completion of the combined Futures.

-        final Promise<PrimaryShardInfo> returnPromise = akka.dispatch.Futures.promise();
+        final Promise<PrimaryShardInfo> returnPromise = Futures.promise();

 
         final OnComplete onComplete = new OnComplete() {
             @Override
 
         final OnComplete onComplete = new OnComplete() {
             @Override


@@ -224,6 +257,42 @@ final class TransactionChainProxy extends AbstractTransactionContextFactory<Loca
         return returnPromise.future();
     }
 
         return returnPromise.future();
     }
 

+    private <T> Future<T> combineFutureWithPossiblePriorReadOnlyTxFutures(final Future<T> future,
+            final TransactionIdentifier txId) {
+        if(!priorReadOnlyTxPromises.containsKey(txId) && !priorReadOnlyTxPromises.isEmpty()) {
+            Collection<Entry<TransactionIdentifier, Promise<Object>>> priorReadOnlyTxPromiseEntries =
+                    new ArrayList<>(priorReadOnlyTxPromises.entrySet());
+            if(priorReadOnlyTxPromiseEntries.isEmpty()) {
+                return future;
+            }
+
+            List<Future<Object>> priorReadOnlyTxFutures = new ArrayList<>(priorReadOnlyTxPromiseEntries.size());
+            for(Entry<TransactionIdentifier, Promise<Object>> entry: priorReadOnlyTxPromiseEntries) {
+                LOG.debug("Tx: {} - waiting on future for prior read-only Tx {}", txId, entry.getKey());
+                priorReadOnlyTxFutures.add(entry.getValue().future());
+            }
+
+            Future<Iterable<Object>> combinedFutures = Futures.sequence(priorReadOnlyTxFutures,
+                    getActorContext().getClientDispatcher());
+
+            final Promise<T> returnPromise = Futures.promise();
+            final OnComplete<Iterable<Object>> onComplete = new OnComplete<Iterable<Object>>() {
+                @Override
+                public void onComplete(final Throwable failure, final Iterable<Object> notUsed) {
+                    LOG.debug("Tx: {} - prior read-only Tx futures complete", txId);
+
+                    // Complete the returned Promise with the original Future.
+                    returnPromise.completeWith(future);
+                }
+            };
+
+            combinedFutures.onComplete(onComplete, getActorContext().getClientDispatcher());
+            return returnPromise.future();
+        } else {
+            return future;
+        }
+    }
+

     @Override
     protected <T> void onTransactionReady(final TransactionIdentifier transaction, final Collection<Future<T>> cohortFutures) {
         final State localState = currentState;
     @Override
     protected <T> void onTransactionReady(final TransactionIdentifier transaction, final Collection<Future<T>> cohortFutures) {
         final State localState = currentState;


@@ -238,8 +307,7 @@ final class TransactionChainProxy extends AbstractTransactionContextFactory<Loca
         }
 
         // Combine the ready Futures into 1
         }
 
         // Combine the ready Futures into 1

-        final Future<Iterable<T>> combined = akka.dispatch.Futures.sequence(
-                cohortFutures, getActorContext().getClientDispatcher());
+        final Future<Iterable<T>> combined = Futures.sequence(cohortFutures, getActorContext().getClientDispatcher());

 
         // Record the we have outstanding futures
         final State newState = new Submitted(transaction, combined);
 
         // Record the we have outstanding futures
         final State newState = new Submitted(transaction, combined);


@@ -255,6 +323,14 @@ final class TransactionChainProxy extends AbstractTransactionContextFactory<Loca
         }, getActorContext().getClientDispatcher());
     }
 
         }, getActorContext().getClientDispatcher());
     }
 

+    @Override
+    protected void onTransactionContextCreated(@Nonnull TransactionIdentifier transactionId) {
+        Promise<Object> promise = priorReadOnlyTxPromises.remove(transactionId);
+        if(promise != null) {
+            promise.success(null);
+        }
+    }
+

     @Override
     protected TransactionIdentifier nextIdentifier() {
         return transactionChainId.newTransactionIdentifier();
     @Override
     protected TransactionIdentifier nextIdentifier() {
         return transactionChainId.newTransactionIdentifier();

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/TransactionContextFactory.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/TransactionContextFactory.java
index 1d141aec2e87135f1ee8fdf2943699a933668d79..db8dedcf353c3450e5475dc5cb2c8723dd401f0a 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/TransactionContextFactory.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/TransactionContextFactory.java
@@ -45,7 +45,7 @@ final class TransactionContextFactory extends AbstractTransactionContextFactory<
     }
 
     @Override
     }
 
     @Override

-    protected Future<PrimaryShardInfo> findPrimaryShard(final String shardName, final String txId) {
+    protected Future<PrimaryShardInfo> findPrimaryShard(final String shardName, TransactionIdentifier txId) {

         return getActorContext().findPrimaryShardAsync(shardName);
     }
 
         return getActorContext().findPrimaryShardAsync(shardName);
     }
 


@@ -57,4 +57,8 @@ final class TransactionContextFactory extends AbstractTransactionContextFactory<
     DOMStoreTransactionChain createTransactionChain() {
         return new TransactionChainProxy(this);
     }
     DOMStoreTransactionChain createTransactionChain() {
         return new TransactionChainProxy(this);
     }

+
+    @Override
+    protected void onTransactionContextCreated(TransactionIdentifier transactionId) {
+    }

 }
 }

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/DistributedDataStoreIntegrationTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/DistributedDataStoreIntegrationTest.java
index 7acde4268f5f7f334ad7ebae0bb2435f27085086..8585cc6acd3c31487edc6eca685145b2853a6c37 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/DistributedDataStoreIntegrationTest.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/DistributedDataStoreIntegrationTest.java
@@ -989,6 +989,51 @@ public class DistributedDataStoreIntegrationTest {
         }};
     }
 
         }};
     }
 

+    @Test
+    public void testChainWithReadOnlyTxAfterPreviousReady() throws Throwable {
+        new IntegrationTestKit(getSystem(), datastoreContextBuilder) {{
+            DistributedDataStore dataStore = setupDistributedDataStore(
+                    "testChainWithReadOnlyTxAfterPreviousReady", "test-1");
+
+            final DOMStoreTransactionChain txChain = dataStore.createTransactionChain();
+
+            // Create a write tx and submit.
+
+            DOMStoreWriteTransaction writeTx = txChain.newWriteOnlyTransaction();
+            writeTx.write(TestModel.TEST_PATH, ImmutableNodes.containerNode(TestModel.TEST_QNAME));
+            DOMStoreThreePhaseCommitCohort cohort1 = writeTx.ready();
+
+            // Create read-only tx's and issue a read.
+
+            CheckedFuture<Optional<NormalizedNode<?, ?>>, ReadFailedException> readFuture1 =
+                    txChain.newReadOnlyTransaction().read(TestModel.TEST_PATH);
+
+            CheckedFuture<Optional<NormalizedNode<?, ?>>, ReadFailedException> readFuture2 =
+                    txChain.newReadOnlyTransaction().read(TestModel.TEST_PATH);
+
+            // Create another write tx and issue the write.
+
+            DOMStoreWriteTransaction writeTx2 = txChain.newWriteOnlyTransaction();
+            writeTx2.write(TestModel.OUTER_LIST_PATH,
+                    ImmutableNodes.mapNodeBuilder(TestModel.OUTER_LIST_QNAME).build());
+
+            // Ensure the reads succeed.
+
+            assertEquals("isPresent", true, readFuture1.checkedGet(5, TimeUnit.SECONDS).isPresent());
+            assertEquals("isPresent", true, readFuture2.checkedGet(5, TimeUnit.SECONDS).isPresent());
+
+            // Ensure the writes succeed.
+
+            DOMStoreThreePhaseCommitCohort cohort2 = writeTx2.ready();
+
+            doCommit(cohort1);
+            doCommit(cohort2);
+
+            assertEquals("isPresent", true, txChain.newReadOnlyTransaction().read(TestModel.OUTER_LIST_PATH).
+                    checkedGet(5, TimeUnit.SECONDS).isPresent());
+        }};
+    }
+

     @Test
     public void testChainedTransactionFailureWithSingleShard() throws Exception{
         new IntegrationTestKit(getSystem(), datastoreContextBuilder) {{
     @Test
     public void testChainedTransactionFailureWithSingleShard() throws Exception{
         new IntegrationTestKit(getSystem(), datastoreContextBuilder) {{