Eliminate TransactionInvokerImpl.transactionToCommand

This map holds Transaction->Command mapping, but it is always
accessed from a context which directly operates on
pendingTransactions queue.

Eliminate the need for lookups (and general HashMap overhead)
by making pendingTransactions hold a pair of transaction/command.

JIRA: OVSDB-428
Change-Id: Ic3040f770a2b3391e188b3ffa7a693776d0058b7
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/transactions/md/TransactionInvokerImpl.java b/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/transactions/md/TransactionInvokerImpl.java
index 4c78ecefd1610c62b08d97e5d70aab520103b6dc..3b469cd06abcbd77714716fc13277f69fa5cce0f 100644 (file)
--- a/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/transactions/md/TransactionInvokerImpl.java
+++ b/southbound/southbound-impl/src/main/java/org/opendaylight/ovsdb/southbound/transactions/md/TransactionInvokerImpl.java
@@ -12,13 +12,13 @@ import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import java.util.AbstractMap.SimpleImmutableEntry;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
-import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Queue;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.ExecutorService;
@@ -49,9 +49,7 @@ public class TransactionInvokerImpl implements TransactionInvoker,TransactionCha
     private final AtomicBoolean runTask = new AtomicBoolean(true);
 
     @GuardedBy("this")
-    private final Map<ReadWriteTransaction, TransactionCommand> transactionToCommand = new HashMap<>();
-    @GuardedBy("this")
-    private final Queue<ReadWriteTransaction> pendingTransactions = new ArrayDeque<>();
+    private final Queue<Entry<ReadWriteTransaction, TransactionCommand>> pendingTransactions = new ArrayDeque<>();
 
     private BindingTransactionChain chain;
 
@@ -71,20 +69,20 @@ public class TransactionInvokerImpl implements TransactionInvoker,TransactionCha
     }
 
     @VisibleForTesting
-    TransactionInvokerImpl(final DataBroker db, final List<ReadWriteTransaction> pendingTransactions,
-            final List<ReadWriteTransaction> failedTransactions,
-            final Map<ReadWriteTransaction, TransactionCommand> transactionToCommand) {
+    TransactionInvokerImpl(final DataBroker db,
+            final List<Entry<ReadWriteTransaction, TransactionCommand>> pendingTransactions,
+            final List<ReadWriteTransaction> failedTransactions) {
         this(db, (ExecutorService) null);
 
         // Initialize state
         this.pendingTransactions.addAll(pendingTransactions);
         this.failedTransactionQueue.addAll(failedTransactions);
-        this.transactionToCommand.putAll(transactionToCommand);
     }
 
     @VisibleForTesting
-    TransactionInvokerImpl(final DataBroker db, final List<ReadWriteTransaction> pendingTransactions) {
-        this(db, pendingTransactions, Collections.emptyList(), Collections.emptyMap());
+    TransactionInvokerImpl(final DataBroker db,
+            final List<Entry<ReadWriteTransaction, TransactionCommand>> pendingTransactions) {
+        this(db, pendingTransactions, Collections.emptyList());
     }
 
     @Override
@@ -166,13 +164,13 @@ public class TransactionInvokerImpl implements TransactionInvoker,TransactionCha
         List<TransactionCommand> commands = new ArrayList<>();
         if (transaction != null) {
             // Process all pending transactions, looking for the failed one...
-            final Iterator<ReadWriteTransaction> it = pendingTransactions.iterator();
+            final Iterator<Entry<ReadWriteTransaction, TransactionCommand>> it = pendingTransactions.iterator();
             while (it.hasNext()) {
-                final ReadWriteTransaction current = it.next();
-                if (transaction.equals(current)) {
-                    // .. collect current and all remaining pending transactions
-                    commands.add(transactionToCommand.get(current));
-                    it.forEachRemaining(tx -> commands.add(transactionToCommand.get(tx)));
+                final Entry<ReadWriteTransaction, TransactionCommand> current = it.next();
+                if (transaction.equals(current.getKey())) {
+                    // .. collect current and all remaining pending transactions' values
+                    commands.add(current.getValue());
+                    it.forEachRemaining(entry -> commands.add(entry.getValue()));
                     break;
                 }
             }
@@ -187,20 +185,24 @@ public class TransactionInvokerImpl implements TransactionInvoker,TransactionCha
         chain.close();
         chain = db.createTransactionChain(this);
         pendingTransactions.clear();
-        transactionToCommand.clear();
         failedTransactionQueue.clear();
     }
 
     synchronized void forgetSuccessfulTransaction(final ReadWriteTransaction transaction) {
-        pendingTransactions.remove(transaction);
-        transactionToCommand.remove(transaction);
+        Iterator<Entry<ReadWriteTransaction, TransactionCommand>> it = pendingTransactions.iterator();
+        while (it.hasNext()) {
+            final Entry<ReadWriteTransaction, TransactionCommand> entry = it.next();
+            if (transaction.equals(entry.getKey())) {
+                it.remove();
+                break;
+            }
+        }
     }
 
     @VisibleForTesting
     synchronized void recordPendingTransaction(final TransactionCommand command,
             final ReadWriteTransaction transaction) {
-        transactionToCommand.put(transaction, command);
-        pendingTransactions.add(transaction);
+        pendingTransactions.add(new SimpleImmutableEntry<>(transaction, command));
     }
 
     @VisibleForTesting

diff --git a/southbound/southbound-impl/src/test/java/org/opendaylight/ovsdb/southbound/transactions/md/TransactionInvokerImplTest.java b/southbound/southbound-impl/src/test/java/org/opendaylight/ovsdb/southbound/transactions/md/TransactionInvokerImplTest.java
index 06a52f760a17ec6527d5c9e912297b6d99944a69..a676347a2148c9d86fb96fdb0088a74bf73216da 100644 (file)
--- a/southbound/southbound-impl/src/test/java/org/opendaylight/ovsdb/southbound/transactions/md/TransactionInvokerImplTest.java
+++ b/southbound/southbound-impl/src/test/java/org/opendaylight/ovsdb/southbound/transactions/md/TransactionInvokerImplTest.java
@@ -9,6 +9,7 @@ package org.opendaylight.ovsdb.southbound.transactions.md;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doNothing;
@@ -18,10 +19,11 @@ import static org.mockito.Mockito.verify;
 import static org.powermock.reflect.Whitebox.getInternalState;
 
 import com.google.common.collect.ImmutableList;
+import java.util.AbstractMap.SimpleImmutableEntry;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Queue;
 import java.util.concurrent.ExecutorService;
 import org.junit.Before;
@@ -84,36 +86,38 @@ public class TransactionInvokerImplTest {
         final ReadWriteTransaction tx1 = mock(ReadWriteTransaction.class);
         final ReadWriteTransaction tx2 = mock(ReadWriteTransaction.class);
         final ReadWriteTransaction tx3 = mock(ReadWriteTransaction.class);
-
-        final Map<ReadWriteTransaction,TransactionCommand> transactionToCommand = new HashMap<>();
-        transactionToCommand.put(tx1, mock(TransactionCommand.class));
+        final TransactionCommand cmd1 = mock(TransactionCommand.class);
         final TransactionCommand cmd2 = mock(TransactionCommand.class);
-        transactionToCommand.put(tx2, cmd2);
         final TransactionCommand cmd3 = mock(TransactionCommand.class);
-        transactionToCommand.put(tx3, cmd3);
 
         final TransactionInvokerImpl invoker = new TransactionInvokerImpl(db,
             // Given pending transaction order ...
-            ImmutableList.of(tx1, tx2, tx3),
+            ImmutableList.of(entry(tx1, cmd1), entry(tx2, cmd2), entry(tx3, cmd3)),
             // .. if tx2 fails ...
-            Collections.singletonList(tx2),
-            transactionToCommand);
+            Collections.singletonList(tx2));
 
         // .. we want to replay tx2 and tx3
         assertEquals(ImmutableList.of(cmd2, cmd3), invoker.extractResubmitCommands());
     }
 
+    private static <K, V> Entry<K, V> entry(final K key, final V value) {
+        return new SimpleImmutableEntry<>(key, value);
+    }
+
     @Test
     public void testResetTransactionQueue() {
         final TransactionInvokerImpl invoker = new TransactionInvokerImpl(db, Collections.emptyList(),
-            Collections.singletonList(mock(ReadWriteTransaction.class)), Collections.emptyMap());
+            Collections.singletonList(mock(ReadWriteTransaction.class)));
 
         invoker.resetTransactionQueue();
 
-        assertNotNull(getInternalState(invoker, "pendingTransactions"));
-        assertNotNull(getInternalState(invoker, "transactionToCommand"));
-        final Queue<?> failedTransactionQueue = getInternalState(invoker, "failedTransactionQueue");
-        assertEquals(0, failedTransactionQueue.size());
+        assertEmpty(getInternalState(invoker, "pendingTransactions"));
+        assertEmpty(getInternalState(invoker, "failedTransactionQueue"));
+    }
+
+    private static void assertEmpty(final Collection<?> collection) {
+        assertNotNull(collection);
+        assertEquals(0, collection.size());
     }
 
     @Test
@@ -124,11 +128,10 @@ public class TransactionInvokerImplTest {
         final ReadWriteTransaction transaction = mock(ReadWriteTransaction.class);
         invoker.recordPendingTransaction(command, transaction);
 
-        Queue<ReadWriteTransaction> testPendingTransactions = getInternalState(invoker, "pendingTransactions");
-        assertEquals(1, testPendingTransactions.size());
-        assertTrue(testPendingTransactions.contains(transaction));
-
-        assertEquals(Collections.singletonMap(transaction, command), getInternalState(invoker, "transactionToCommand"));
+        Queue<Entry<?, ?>> endingTransactions = getInternalState(invoker, "pendingTransactions");
+        assertEquals(1, endingTransactions.size());
+        assertSame(transaction, endingTransactions.element().getKey());
+        assertSame(command, endingTransactions.element().getValue());
     }
 
     @Test