Move OperationLimiter.acquire() warnings to callers 01/68401/4
authorRobert Varga <robert.varga@pantheon.tech>
Tue, 23 Jan 2018 16:42:44 +0000 (17:42 +0100)
committerRobert Varga <robert.varga@pantheon.tech>
Wed, 28 Feb 2018 17:37:17 +0000 (18:37 +0100)
We have two distinct call sites from where we are acquiring permits,
where failure to acquire has different root cause:
RemoteTransactionContext's failure to acquire indicates a slow leader,
while TransactionContextWrapper indicates a failure to discover a leader
in time.

Modify acquire() to report success and log failures to acquire from
the two call sites, with different messages so it is immediately clear
what is going on.

Change-Id: Ie828984c6a984690224494af1a9fdd0e4f257fab
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/AbstractTransactionContextFactory.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/OperationLimiter.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/RemoteTransactionContext.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/TransactionContextWrapper.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/TransactionContextWrapperTest.java

index 69c69bb..70a014f 100644 (file)
@@ -116,7 +116,7 @@ abstract class AbstractTransactionContextFactory<F extends LocalTransactionFacto
     final TransactionContextWrapper newTransactionContextWrapper(final TransactionProxy parent,
             final String shardName) {
         final TransactionContextWrapper transactionContextWrapper =
-                new TransactionContextWrapper(parent.getIdentifier(), actorContext);
+                new TransactionContextWrapper(parent.getIdentifier(), actorContext, shardName);
 
         Future<PrimaryShardInfo> findPrimaryFuture = findPrimaryShard(shardName, parent.getIdentifier());
         if (findPrimaryFuture.isCompleted()) {
index ea93f1f..e1fee6d 100644 (file)
@@ -45,8 +45,6 @@ public class OperationLimiter  {
             if (semaphore.tryAcquire(acquirePermits, acquireTimeout, TimeUnit.NANOSECONDS)) {
                 return true;
             }
-
-            LOG.warn("Failed to acquire operation permit for transaction {}", identifier);
         } catch (InterruptedException e) {
             if (LOG.isDebugEnabled()) {
                 LOG.debug("Interrupted when trying to acquire operation permit for transaction {}", identifier, e);
@@ -66,7 +64,8 @@ public class OperationLimiter  {
         this.semaphore.release(permits);
     }
 
-    public TransactionIdentifier getIdentifier() {
+    @VisibleForTesting
+    TransactionIdentifier getIdentifier() {
         return identifier;
     }
 
index 7c45c54..941c861 100644 (file)
@@ -245,7 +245,16 @@ public class RemoteTransactionContext extends AbstractTransactionContext {
      * @return True if a permit was successfully acquired, false otherwise
      */
     private boolean acquireOperation() {
-        return isOperationHandOffComplete() && limiter.acquire();
+        if (isOperationHandOffComplete()) {
+            if (limiter.acquire()) {
+                return true;
+            }
+
+            LOG.warn("Failed to acquire execute operation permit for transaction {} on actor {}", getIdentifier(),
+                actor);
+        }
+
+        return false;
     }
 
     @Override
index 3fb129f..a126ce9 100644 (file)
@@ -38,8 +38,8 @@ class TransactionContextWrapper {
      */
     @GuardedBy("queuedTxOperations")
     private final List<TransactionOperation> queuedTxOperations = Lists.newArrayList();
-
     private final TransactionIdentifier identifier;
+    private final String shardName;
 
     /**
      * The resulting TransactionContext.
@@ -48,12 +48,14 @@ class TransactionContextWrapper {
 
     private final OperationLimiter limiter;
 
-    TransactionContextWrapper(final TransactionIdentifier identifier, final ActorContext actorContext) {
+    TransactionContextWrapper(final TransactionIdentifier identifier, final ActorContext actorContext,
+            final String shardName) {
         this.identifier = Preconditions.checkNotNull(identifier);
         this.limiter = new OperationLimiter(identifier,
                 // 1 extra permit for the ready operation
                 actorContext.getDatastoreContext().getShardBatchedModificationCount() + 1,
                 TimeUnit.MILLISECONDS.toSeconds(actorContext.getDatastoreContext().getOperationTimeoutInMillis()));
+        this.shardName = Preconditions.checkNotNull(shardName);
     }
 
     TransactionContext getTransactionContext() {
@@ -71,7 +73,7 @@ class TransactionContextWrapper {
         final boolean invokeOperation;
         synchronized (queuedTxOperations) {
             if (transactionContext == null) {
-                LOG.debug("Tx {} Queuing TransactionOperation", getIdentifier());
+                LOG.debug("Tx {} Queuing TransactionOperation", identifier);
 
                 queuedTxOperations.add(operation);
                 invokeOperation = false;
@@ -83,7 +85,10 @@ class TransactionContextWrapper {
         if (invokeOperation) {
             operation.invoke(transactionContext);
         } else {
-            limiter.acquire();
+            if (!limiter.acquire()) {
+                LOG.warn("Failed to acquire enqueue operation permit for transaction {} on shard {}", identifier,
+                    shardName);
+            }
         }
     }
 
index 058968c..888a9e6 100644 (file)
@@ -32,7 +32,7 @@ public class TransactionContextWrapperTest {
         MockitoAnnotations.initMocks(this);
         doReturn(DatastoreContext.newBuilder().build()).when(actorContext).getDatastoreContext();
         transactionContextWrapper = new TransactionContextWrapper(MockIdentifiers.transactionIdentifier(
-            TransactionContextWrapperTest.class, "mock"), actorContext);
+            TransactionContextWrapperTest.class, "mock"), actorContext, "mock");
     }
 
     @Test

©2013 OpenDaylight, A Linux Foundation Collaborative Project. All Rights Reserved.
OpenDaylight is a registered trademark of The OpenDaylight Project, Inc.
Linux Foundation and OpenDaylight are registered trademarks of the Linux Foundation.
Linux is a registered trademark of Linus Torvalds.