From: Tony Tkacik Date: Mon, 31 Aug 2015 08:59:12 +0000 (+0200) Subject: Bug 3869: Fixed ShardedDOMDataTree to adhere to API X-Git-Tag: release/beryllium~115 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=2a9303a2358f30d8ff5e50cece0d6f421d9d2c64;p=mdsal.git 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 Signed-off-by: Robert Varga --- 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 53a3977448..d11ea898de 100644 --- 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 7566b93fb7..ac5f1f77d1 100644 --- 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 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 7841e43209..82966e9147 100644 --- 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 { private static final Logger LOG = LoggerFactory.getLogger(ShardingTableEntry.class); - private final Map children = Collections.emptyMap(); + // FIXME: We do probably want to adapt map + private final Map children = new HashMap<>(); private final PathArgument identifier; private ShardRegistration registration; @@ -69,6 +70,8 @@ final class ShardingTableEntry implements Identifiable { 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 { } 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 index 0000000000..7f81e1d5fb --- /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 SUBTREES_ROOT = Collections.singleton(ROOT_ID); + private static final Collection SUBTREES_TEST = Collections.singleton(TEST_ID); + + private static final DOMStoreThreePhaseCommitCohort ALLWAYS_SUCCESS = new DOMStoreThreePhaseCommitCohort() { + + @Override + public ListenableFuture preCommit() { + return Futures.immediateFuture(null); + } + + @Override + public ListenableFuture commit() { + return Futures.immediateFuture(null); + } + + @Override + public ListenableFuture canCommit() { + return Futures.immediateFuture(Boolean.TRUE); + } + + @Override + public ListenableFuture 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 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.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 index 0000000000..8614a41178 --- /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 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 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(); + } + +}