Bug 3869: Fixed ShardedDOMDataTree to adhere to API

ShardingTableEntry:
 - Replaced emptyMap with HashMap to allow to populate
   table entry
 - Fixed #remove(Iterator) to remove registration

ShardedDOMWriteTransaction:
 - Checks if writen path is delegated to other child
   producer.
 - submit() invokes DOMDataTreeProducer in order
   to notify when transaction is submitted.

ShardedDOMDataTreeProducer
 - #transactionSubmitted() clears openTx field
   in order to allow to allocate next transaction
   or close producer.

Added unit tests which tests ShardedDOMDataTree
to adhere to Shard & Producer API contracts defined
in DOMDataTreeService and DOMDataTreeProducerService

Change-Id: I39fae5f40e5b8a1cd07fcbe183c937b5c5bda348
Signed-off-by: Tony Tkacik <ttkacik@cisco.com>
Signed-off-by: Robert Varga <rovarga@cisco.com>

diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeProducer.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeProducer.java
index 53a39774489dd84780e6f593f31977d62e2a9b1d..d11ea898dec1806595e79363c7fc5ff201c46869 100644 (file)
--- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeProducer.java
+++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTreeProducer.java
@@ -7,16 +7,6 @@
  */
 package org.opendaylight.mdsal.dom.broker;
 
-import org.opendaylight.mdsal.dom.spi.store.DOMStore;
-import org.opendaylight.mdsal.dom.spi.store.DOMStoreTransactionChain;
-import org.opendaylight.mdsal.dom.spi.store.DOMStoreWriteTransaction;
-
-import org.opendaylight.mdsal.dom.api.DOMDataTreeIdentifier;
-import org.opendaylight.mdsal.dom.api.DOMDataTreeProducer;
-import org.opendaylight.mdsal.dom.api.DOMDataTreeProducerBusyException;
-import org.opendaylight.mdsal.dom.api.DOMDataTreeProducerException;
-import org.opendaylight.mdsal.dom.api.DOMDataTreeShard;
-import org.opendaylight.mdsal.dom.api.DOMDataWriteTransaction;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.BiMap;
 import com.google.common.collect.ImmutableBiMap;
@@ -32,6 +22,15 @@ import java.util.Map.Entry;
 import java.util.Queue;
 import java.util.Set;
 import javax.annotation.concurrent.GuardedBy;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeIdentifier;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeProducer;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeProducerBusyException;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeProducerException;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeShard;
+import org.opendaylight.mdsal.dom.api.DOMDataWriteTransaction;
+import org.opendaylight.mdsal.dom.spi.store.DOMStore;
+import org.opendaylight.mdsal.dom.spi.store.DOMStoreTransactionChain;
+import org.opendaylight.mdsal.dom.spi.store.DOMStoreWriteTransaction;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -143,10 +142,7 @@ final class ShardedDOMDataTreeProducer implements DOMDataTreeProducer {
 
         for (final DOMDataTreeIdentifier s : subtrees) {
             // Check if the subtree was visible at any time
-            if (!haveSubtree(s)) {
-                throw new IllegalArgumentException(String.format("Subtree %s was never available in producer %s", s, this));
-            }
-
+            Preconditions.checkArgument(haveSubtree(s), "Subtree %s was never available in producer %s", s, this);
             // Check if the subtree has not been delegated to a child
             final DOMDataTreeProducer child = lookupChild(s);
             Preconditions.checkArgument(child == null, "Subtree %s is delegated to child producer %s", s, child);
@@ -170,6 +166,16 @@ final class ShardedDOMDataTreeProducer implements DOMDataTreeProducer {
         return ret;
     }
 
+    boolean isDelegatedToChild(DOMDataTreeIdentifier path) {
+        for (final DOMDataTreeIdentifier c : children.keySet()) {
+            if (c.contains(path)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+
     @Override
     public synchronized void close() throws DOMDataTreeProducerException {
         if (!closed) {
@@ -208,4 +214,9 @@ final class ShardedDOMDataTreeProducer implements DOMDataTreeProducer {
         LOG.debug("Transaction {} cancelled", transaction);
         openTx = null;
     }
+
+    synchronized void transactionSubmitted(ShardedDOMDataWriteTransaction transaction) {
+        Preconditions.checkState(openTx.equals(transaction));
+        openTx = null;
+    }
 }

diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataWriteTransaction.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataWriteTransaction.java
index 7566b93fb70745e3907a1175bce046a280e310ea..ac5f1f77d1dfe9f38df4794425c6261d34bca314 100644 (file)
--- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataWriteTransaction.java
+++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataWriteTransaction.java
@@ -7,11 +7,6 @@
  */
 package org.opendaylight.mdsal.dom.broker;
 
-import org.opendaylight.mdsal.dom.spi.store.DOMStoreThreePhaseCommitCohort;
-import org.opendaylight.mdsal.dom.spi.store.DOMStoreWriteTransaction;
-
-import org.opendaylight.mdsal.common.api.LogicalDatastoreType;
-import org.opendaylight.mdsal.common.api.TransactionCommitFailedException;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.util.concurrent.CheckedFuture;
@@ -24,8 +19,12 @@ import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
 import javax.annotation.concurrent.GuardedBy;
 import javax.annotation.concurrent.NotThreadSafe;
+import org.opendaylight.mdsal.common.api.LogicalDatastoreType;
+import org.opendaylight.mdsal.common.api.TransactionCommitFailedException;
 import org.opendaylight.mdsal.dom.api.DOMDataTreeIdentifier;
 import org.opendaylight.mdsal.dom.api.DOMDataWriteTransaction;
+import org.opendaylight.mdsal.dom.spi.store.DOMStoreThreePhaseCommitCohort;
+import org.opendaylight.mdsal.dom.spi.store.DOMStoreWriteTransaction;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
 import org.slf4j.Logger;
@@ -54,10 +53,12 @@ final class ShardedDOMDataWriteTransaction implements DOMDataWriteTransaction {
 
         for (final Entry<DOMDataTreeIdentifier, DOMStoreWriteTransaction> e : idToTransaction.entrySet()) {
             if (e.getKey().contains(id)) {
+                Preconditions.checkArgument(!producer.isDelegatedToChild(id),
+                        "Path %s is delegated to child producer.",
+                        id);
                 return e.getValue();
             }
         }
-
         throw new IllegalArgumentException(String.format("Path %s is not acessible from transaction %s", id, this));
     }
 
@@ -91,7 +92,7 @@ final class ShardedDOMDataWriteTransaction implements DOMDataWriteTransaction {
         for (final DOMStoreWriteTransaction tx : txns) {
             cohorts.add(tx.ready());
         }
-
+        producer.transactionSubmitted(this);
         try {
             return Futures.immediateCheckedFuture(new CommitCoordinationTask(this, cohorts, null).call());
         } catch (final TransactionCommitFailedException e) {

diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardingTableEntry.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardingTableEntry.java
index 7841e432096fe24e7c0f7cd4acfdc5f9a19cfcdb..82966e9147771d3e210bf2e4f04ae7006d435b53 100644 (file)
--- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardingTableEntry.java
+++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardingTableEntry.java
@@ -8,7 +8,7 @@
 package org.opendaylight.mdsal.dom.broker;
 
 import com.google.common.base.Preconditions;
-import java.util.Collections;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
 import org.opendaylight.yangtools.concepts.Identifiable;
@@ -19,7 +19,8 @@ import org.slf4j.LoggerFactory;
 
 final class ShardingTableEntry implements Identifiable<PathArgument> {
     private static final Logger LOG = LoggerFactory.getLogger(ShardingTableEntry.class);
-    private final Map<PathArgument, ShardingTableEntry> children = Collections.emptyMap();
+    // FIXME: We do probably want to adapt map
+    private final Map<PathArgument, ShardingTableEntry> children = new HashMap<>();
     private final PathArgument identifier;
     private ShardRegistration<?> registration;
 
@@ -69,6 +70,8 @@ final class ShardingTableEntry implements Identifiable<PathArgument> {
                 child = new ShardingTableEntry(a);
                 entry.children.put(a, child);
             }
+            // TODO: Is this correct? We want to enter child
+            entry = child;
         }
 
         Preconditions.checkState(entry.registration == null);
@@ -86,8 +89,14 @@ final class ShardingTableEntry implements Identifiable<PathArgument> {
             } else {
                 LOG.warn("Cannot remove non-existent child {}", arg);
             }
+        } else {
+            /*
+             * Iterator is empty, this effectivelly means is table entry to remove registration.
+             * FIXME: We probably want to compare registration object to make sure we are removing
+             * correct shard.
+             */
+            registration = null;
         }
-
         return registration == null && children.isEmpty();
     }
 

diff --git a/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/test/ShardedDOMDataTreeProducerSingleShardTest.java b/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/test/ShardedDOMDataTreeProducerSingleShardTest.java
new file mode 100644 (file)
index 0000000..7f81e1d
--- /dev/null
+++ b/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/test/ShardedDOMDataTreeProducerSingleShardTest.java
@@ -0,0 +1,167 @@
+/*
+ * Copyright (c) 2015 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.mdsal.dom.broker.test;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.doReturn;
+
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+import java.util.Collection;
+import java.util.Collections;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.opendaylight.mdsal.common.api.LogicalDatastoreType;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeIdentifier;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeProducer;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeProducerBusyException;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeProducerException;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeService;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeShard;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeShardingConflictException;
+import org.opendaylight.mdsal.dom.api.DOMDataWriteTransaction;
+import org.opendaylight.mdsal.dom.broker.ShardedDOMDataTree;
+import org.opendaylight.mdsal.dom.broker.test.util.TestModel;
+import org.opendaylight.mdsal.dom.spi.store.DOMStore;
+import org.opendaylight.mdsal.dom.spi.store.DOMStoreThreePhaseCommitCohort;
+import org.opendaylight.mdsal.dom.spi.store.DOMStoreTransactionChain;
+import org.opendaylight.mdsal.dom.spi.store.DOMStoreWriteTransaction;
+import org.opendaylight.yangtools.concepts.ListenerRegistration;
+import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
+import org.opendaylight.yangtools.yang.data.impl.schema.ImmutableNodes;
+
+public class ShardedDOMDataTreeProducerSingleShardTest {
+
+
+    private static final DOMDataTreeIdentifier ROOT_ID = new DOMDataTreeIdentifier(LogicalDatastoreType.OPERATIONAL,
+            YangInstanceIdentifier.EMPTY);
+    private static final DOMDataTreeIdentifier TEST_ID = new DOMDataTreeIdentifier(LogicalDatastoreType.OPERATIONAL,
+            TestModel.TEST_PATH);
+
+    private static final Collection<DOMDataTreeIdentifier> SUBTREES_ROOT = Collections.singleton(ROOT_ID);
+    private static final Collection<DOMDataTreeIdentifier> SUBTREES_TEST = Collections.singleton(TEST_ID);
+
+    private static final DOMStoreThreePhaseCommitCohort ALLWAYS_SUCCESS = new DOMStoreThreePhaseCommitCohort() {
+
+        @Override
+        public ListenableFuture<Void> preCommit() {
+            return Futures.immediateFuture(null);
+        }
+
+        @Override
+        public ListenableFuture<Void> commit() {
+            return Futures.immediateFuture(null);
+        }
+
+        @Override
+        public ListenableFuture<Boolean> canCommit() {
+            return Futures.immediateFuture(Boolean.TRUE);
+        }
+
+        @Override
+        public ListenableFuture<Void> abort() {
+            return Futures.immediateFuture(null);
+        }
+    };
+
+    interface MockTestShard extends DOMDataTreeShard, DOMStore {
+
+    }
+
+
+    @Mock(name = "rootShard")
+    private MockTestShard rootShard;
+
+
+    @Mock(name = "storeWriteTx")
+    private DOMStoreWriteTransaction writeTxMock;
+
+    @Mock(name = "storeTxChain")
+    private DOMStoreTransactionChain txChainMock;
+
+
+
+    private DOMDataTreeService treeService;
+    private ListenerRegistration<MockTestShard> shardReg;
+    private DOMDataTreeProducer producer;
+
+
+
+
+    @Before
+    public void setUp() throws DOMDataTreeShardingConflictException {
+        MockitoAnnotations.initMocks(this);
+        final ShardedDOMDataTree impl = new ShardedDOMDataTree();
+        treeService = impl;
+        shardReg = impl.registerDataTreeShard(ROOT_ID, rootShard);
+
+        doReturn("rootShard").when(rootShard).toString();
+        doReturn(txChainMock).when(rootShard).createTransactionChain();
+        doReturn(writeTxMock).when(txChainMock).newWriteOnlyTransaction();
+        doReturn(ALLWAYS_SUCCESS).when(writeTxMock).ready();
+
+        producer = treeService.createProducer(SUBTREES_ROOT);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void createProducerWithEmptyList() {
+        treeService.createProducer(Collections.<DOMDataTreeIdentifier>emptySet());
+    }
+
+    @Test(expected = DOMDataTreeProducerBusyException.class)
+    public void closeWithTxOpened() throws DOMDataTreeProducerException {
+        producer.createTransaction(false);
+        producer.close();
+    }
+
+    @Test
+    public void closeWithTxSubmitted() throws DOMDataTreeProducerException {
+        DOMDataWriteTransaction tx = producer.createTransaction(false);
+        tx.submit();
+        producer.close();
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void allocateTxWithTxOpen() {
+        producer.createTransaction(false);
+        producer.createTransaction(false);
+    }
+
+
+    @Test(expected = IllegalStateException.class)
+    public void allocateChildProducerWithTxOpen() {
+        producer.createTransaction(false);
+        producer.createProducer(SUBTREES_TEST);
+    }
+
+    @Test
+    public void allocateChildProducerWithTxSubmmited() {
+        producer.createTransaction(false).submit();
+        DOMDataTreeProducer childProducer = producer.createProducer(SUBTREES_TEST);
+        assertNotNull(childProducer);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void writeChildProducerDataToParentTx() {
+        DOMDataTreeProducer childProducer = producer.createProducer(SUBTREES_TEST);
+        assertNotNull(childProducer);
+        DOMDataWriteTransaction parentTx = producer.createTransaction(true);
+        parentTx.put(TEST_ID.getDatastoreType(), TEST_ID.getRootIdentifier(),
+                ImmutableNodes.containerNode(TestModel.TEST_QNAME));
+    }
+
+    @Test
+    public void allocateTxWithTxSubmitted() {
+        producer.createTransaction(false).submit();
+        producer.createTransaction(false);
+    }
+
+}

diff --git a/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/test/ShardedDOMDataTreeShardTest.java b/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/test/ShardedDOMDataTreeShardTest.java
new file mode 100644 (file)
index 0000000..8614a41
--- /dev/null
+++ b/dom/mdsal-dom-broker/src/test/java/org/opendaylight/mdsal/dom/broker/test/ShardedDOMDataTreeShardTest.java
@@ -0,0 +1,82 @@
+/*
+ * Copyright (c) 2015 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.mdsal.dom.broker.test;
+
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.opendaylight.mdsal.common.api.LogicalDatastoreType;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeIdentifier;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeShard;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeShardingConflictException;
+import org.opendaylight.mdsal.dom.api.DOMDataTreeShardingService;
+import org.opendaylight.mdsal.dom.broker.ShardedDOMDataTree;
+import org.opendaylight.mdsal.dom.broker.test.util.TestModel;
+import org.opendaylight.yangtools.concepts.ListenerRegistration;
+import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
+
+public class ShardedDOMDataTreeShardTest {
+
+
+    private static final DOMDataTreeIdentifier ROOT_ID = new DOMDataTreeIdentifier(LogicalDatastoreType.OPERATIONAL,
+            YangInstanceIdentifier.EMPTY);
+    private static final DOMDataTreeIdentifier TEST_ID = new DOMDataTreeIdentifier(LogicalDatastoreType.OPERATIONAL,
+            TestModel.TEST_PATH);
+
+    @Mock(name = "rootShard")
+    private DOMDataTreeShard rootShard;
+
+    @Mock(name = "rootShard")
+    private DOMDataTreeShard childShard;
+
+    private DOMDataTreeShardingService shardingService;
+    private ListenerRegistration<DOMDataTreeShard> shardReg;
+
+    @Before
+    public void setUp() throws DOMDataTreeShardingConflictException {
+        MockitoAnnotations.initMocks(this);
+        final ShardedDOMDataTree impl = new ShardedDOMDataTree();
+        shardingService = impl;
+        shardReg = impl.registerDataTreeShard(ROOT_ID, rootShard);
+        doReturn("rootShard").when(rootShard).toString();
+        doReturn("childShard").when(childShard).toString();
+    }
+
+    @Test
+    public void attachChildShard() throws DOMDataTreeShardingConflictException {
+        doNothing().when(rootShard).onChildAttached(TEST_ID, childShard);
+        shardingService.registerDataTreeShard(TEST_ID, childShard);
+        verify(rootShard, times(1)).onChildAttached(TEST_ID, childShard);
+    }
+
+    @Test
+    public void attachAndRemoveShard() throws DOMDataTreeShardingConflictException {
+        doNothing().when(rootShard).onChildAttached(TEST_ID, childShard);
+        ListenerRegistration<DOMDataTreeShard> reg = shardingService.registerDataTreeShard(TEST_ID, childShard);
+        verify(rootShard, times(1)).onChildAttached(TEST_ID, childShard);
+
+        doNothing().when(rootShard).onChildDetached(TEST_ID, childShard);
+        doNothing().when(childShard).onChildDetached(TEST_ID, childShard);
+
+        reg.close();
+        verify(rootShard, times(1)).onChildDetached(TEST_ID, childShard);
+    }
+
+    @Test
+    public void removeShard() {
+        doNothing().when(rootShard).onChildDetached(ROOT_ID, rootShard);
+        shardReg.close();
+    }
+
+}