From 35563cb3c795ab92d0e85c34e3b3bab5da0c73bc Mon Sep 17 00:00:00 2001 From: Tony Tkacik Date: Wed, 23 Apr 2014 16:17:31 +0200 Subject: [PATCH] Bug 509: Added support for merge operation to InMemoryData Store Merge operation support is critical for backwards compatibility and for Binding-Aware mapping and Restconf, which are not aware of Mixin nodes representing lists, leaflists and choices. Semantics of merge operation are similar to subtree modified, but when node is not existing it creates one based on state provided by user of the Data Broker APIs. Merge operation also fixes concurrent creations of mixin nodes as demonstrated in ConcurrentImplicitCreateTest, where two separate applications may indirectly create same root node. Change-Id: I9ab290b512601bd99750a68ee53564e67ef0c879 Signed-off-by: Tony Tkacik --- .../impl/AbstractForwardedTransaction.java | 2 +- .../data/ConcurrentImplicitCreateTest.java | 60 +++++++++++++++++++ .../dom/broker/impl/DOMDataBrokerImpl.java | 9 +-- .../BackwardsCompatibleTransaction.java | 52 +--------------- .../dom/store/impl/InMemoryDOMDataStore.java | 12 ++++ .../sal/dom/store/impl/MutableDataTree.java | 19 ++++++ .../store/impl/OperationWithModification.java | 22 +++++++ .../impl/ResolveDataChangeEventsTask.java | 2 + .../store/impl/SchemaAwareApplyOperation.java | 42 +++++++++++-- .../dom/store/impl/tree/ModificationType.java | 9 ++- .../dom/store/impl/tree/NodeModification.java | 14 ++++- .../spi/data/DOMStoreWriteTransaction.java | 19 ++++++ 12 files changed, 193 insertions(+), 69 deletions(-) create mode 100644 opendaylight/md-sal/sal-binding-dom-it/src/test/java/org/opendaylight/controller/md/sal/binding/data/ConcurrentImplicitCreateTest.java diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/AbstractForwardedTransaction.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/AbstractForwardedTransaction.java index 6334457fd4..6bd6e6aaf5 100644 --- a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/AbstractForwardedTransaction.java +++ b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/AbstractForwardedTransaction.java @@ -132,7 +132,7 @@ public class AbstractForwardedTransaction NODES_PATH = InstanceIdentifier.builder(Nodes.class).build(); + private static InstanceIdentifier NODE_FOO_PATH = InstanceIdentifier.builder(NODES_PATH) + .child(Node.class, NODE_FOO_KEY).build(); + private static InstanceIdentifier NODE_BAR_PATH = InstanceIdentifier.builder(NODES_PATH) + .child(Node.class, NODE_FOO_KEY).build(); + + @Test + public void testConcurrentCreate() throws InterruptedException, ExecutionException { + + DataModificationTransaction fooTx = baDataService.beginTransaction(); + DataModificationTransaction barTx = baDataService.beginTransaction(); + + fooTx.putOperationalData(NODE_FOO_PATH, new NodeBuilder().setKey(NODE_FOO_KEY).build()); + barTx.putOperationalData(NODE_BAR_PATH, new NodeBuilder().setKey(NODE_BAR_KEY).build()); + + Future> fooFuture = fooTx.commit(); + Future> barFuture = barTx.commit(); + + RpcResult fooResult = fooFuture.get(); + RpcResult barResult = barFuture.get(); + + assertTrue(fooResult.isSuccessful()); + assertTrue(barResult.isSuccessful()); + + assertEquals(TransactionStatus.COMMITED, fooResult.getResult()); + assertEquals(TransactionStatus.COMMITED, barResult.getResult()); + + } +} diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMDataBrokerImpl.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMDataBrokerImpl.java index 64a5606e3f..608ac9bc68 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMDataBrokerImpl.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/DOMDataBrokerImpl.java @@ -217,8 +217,7 @@ public class DOMDataBrokerImpl implements DOMDataBroker, AutoCloseable { @Override public void merge(final LogicalDatastoreType store, final InstanceIdentifier path, final NormalizedNode data) { - // TODO Auto-generated method stub - throw new UnsupportedOperationException("Not implemented yet."); + getSubtransaction(store).merge(path,data); } @Override @@ -251,12 +250,6 @@ public class DOMDataBrokerImpl implements DOMDataBroker, AutoCloseable { final InstanceIdentifier path) { return getSubtransaction(store).read(path); } - - @Override - public void merge(final LogicalDatastoreType store, final InstanceIdentifier path, - final NormalizedNode data) { - - } } private final class CommitCoordination implements Callable> { diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/compat/BackwardsCompatibleTransaction.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/compat/BackwardsCompatibleTransaction.java index b905d2f673..29f2af511e 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/compat/BackwardsCompatibleTransaction.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/broker/impl/compat/BackwardsCompatibleTransaction.java @@ -32,21 +32,13 @@ import org.opendaylight.yangtools.concepts.ListenerRegistration; import org.opendaylight.yangtools.yang.common.RpcResult; import org.opendaylight.yangtools.yang.data.api.CompositeNode; import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier; -import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.AugmentationIdentifier; -import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.NodeIdentifier; import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.PathArgument; -import org.opendaylight.yangtools.yang.data.api.schema.AugmentationNode; -import org.opendaylight.yangtools.yang.data.api.schema.LeafSetNode; -import org.opendaylight.yangtools.yang.data.api.schema.MapEntryNode; -import org.opendaylight.yangtools.yang.data.api.schema.MapNode; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; -import org.opendaylight.yangtools.yang.data.impl.schema.Builders; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.google.common.base.Optional; import com.google.common.base.Preconditions; -import com.google.common.collect.Iterables; import com.google.common.util.concurrent.ListenableFuture; public abstract class BackwardsCompatibleTransaction implements @@ -249,7 +241,7 @@ public abstract class BackwardsCompatibleTransaction parentArgs = parentPath(normalizedPath).getPath(); - if(parentArgs.isEmpty()) { - return false; - } - return Iterables.getLast(parentArgs) instanceof AugmentationIdentifier; - } - - private void ensureParentNode(final LogicalDatastoreType store, final InstanceIdentifier normalizedPath, - final NormalizedNode normalizedData) { - InstanceIdentifier parentPath = parentPath(normalizedPath); - PathArgument parentType = Iterables.getLast(parentPath.getPath()); - if(parentType instanceof AugmentationIdentifier) { - AugmentationNode node = Builders.augmentationBuilder() - .withNodeIdentifier((AugmentationIdentifier) parentType) - .build(); - getDelegate().put(store, parentPath, node); - } - if(normalizedData instanceof MapEntryNode) { - MapNode mapNode = Builders.mapBuilder().withNodeIdentifier(new NodeIdentifier(normalizedData.getNodeType())).build(); - getDelegate().put(store, parentPath, mapNode); - } else if (normalizedData instanceof LeafSetNode){ - LeafSetNode leafNode = Builders.leafSetBuilder().withNodeIdentifier(new NodeIdentifier(normalizedData.getNodeType())).build(); - getDelegate().put(store, parentPath, leafNode); - } - - - } - - private InstanceIdentifier parentPath(final InstanceIdentifier normalizedPath) { - List childArgs = normalizedPath.getPath(); - return new InstanceIdentifier(childArgs.subList(0, childArgs.size() -1)); - } - - private boolean parentNodeDoesNotExists(final LogicalDatastoreType store, final InstanceIdentifier normalizedPath) { - try { - return !getDelegate().read(store, parentPath(normalizedPath)).get().isPresent(); - } catch (InterruptedException | ExecutionException e) { - throw new IllegalStateException(e); - } - } - @Override public void removeConfigurationData(final InstanceIdentifier legacyPath) { checkNotNull(legacyPath, "Path MUST NOT be null."); diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/InMemoryDOMDataStore.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/InMemoryDOMDataStore.java index 6c0e5823a4..9bbba1e24d 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/InMemoryDOMDataStore.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/InMemoryDOMDataStore.java @@ -258,6 +258,18 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable, Sch } } + @Override + public void merge(final InstanceIdentifier path, final NormalizedNode data) { + checkNotReady(); + try { + LOG.trace("Tx: {} Merge: {}:{}",getIdentifier(),path,data); + mutableTree.merge(path, data); + // FIXME: Add checked exception + } catch (Exception e) { + LOG.error("Tx: {}, failed to write {}:{} in {}",getIdentifier(),path,data,mutableTree,e); + } + } + @Override public void delete(final InstanceIdentifier path) { checkNotReady(); diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/MutableDataTree.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/MutableDataTree.java index bc7fe7a2c7..b711163b46 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/MutableDataTree.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/MutableDataTree.java @@ -18,6 +18,7 @@ import org.opendaylight.controller.md.sal.dom.store.impl.tree.TreeNodeUtils; import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier; import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.PathArgument; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; +import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNodeContainer; import org.opendaylight.yangtools.yang.data.impl.schema.NormalizedNodeUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -47,6 +48,24 @@ class MutableDataTree { resolveModificationFor(path).write(value); } + public void merge(final InstanceIdentifier path, final NormalizedNode data) { + checkSealed(); + mergeImpl(resolveModificationFor(path),data); + } + + private void mergeImpl(final OperationWithModification op,final NormalizedNode data) { + + if(data instanceof NormalizedNodeContainer) { + @SuppressWarnings({ "rawtypes", "unchecked" }) + NormalizedNodeContainer> dataContainer = (NormalizedNodeContainer) data; + for(NormalizedNode child : dataContainer.getValue()) { + PathArgument childId = child.getIdentifier(); + mergeImpl(op.forChild(childId), child); + } + } + op.merge(data); + } + public void delete(final InstanceIdentifier path) { checkSealed(); resolveModificationFor(path).delete(); diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/OperationWithModification.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/OperationWithModification.java index eaf01aeecd..35864b6bc2 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/OperationWithModification.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/OperationWithModification.java @@ -2,6 +2,7 @@ package org.opendaylight.controller.md.sal.dom.store.impl; import org.opendaylight.controller.md.sal.dom.store.impl.tree.NodeModification; import org.opendaylight.controller.md.sal.dom.store.impl.tree.StoreMetadataNode; +import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.PathArgument; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; import com.google.common.base.Optional; @@ -10,6 +11,7 @@ import com.google.common.primitives.UnsignedLong; public class OperationWithModification { private final NodeModification modification; + private final ModificationApplyOperation applyOperation; private OperationWithModification(final ModificationApplyOperation op, final NodeModification mod) { @@ -28,6 +30,14 @@ public class OperationWithModification { return this; } + public NodeModification getModification() { + return modification; + } + + public ModificationApplyOperation getApplyOperation() { + return applyOperation; + } + public boolean isApplicable(final Optional data) { return applyOperation.isApplicable(modification, data); } @@ -41,4 +51,16 @@ public class OperationWithModification { return new OperationWithModification(operation, modification); } + + public void merge(final NormalizedNode data) { + modification.merge(data); + applyOperation.verifyStructure(modification); + + } + + public OperationWithModification forChild(final PathArgument childId) { + NodeModification childMod = modification.modifyChild(childId); + Optional childOp = applyOperation.getChild(childId); + return from(childOp.get(),childMod); + } } \ No newline at end of file diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeEventsTask.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeEventsTask.java index 520bd1ba1d..c62c27dea8 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeEventsTask.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/ResolveDataChangeEventsTask.java @@ -308,6 +308,7 @@ public class ResolveDataChangeEventsTask implements Callable current) { + Optional original = modification.getOriginal(); + if (original.isPresent() && current.isPresent()) { + return isNotConflicting(original.get(), current.get()); + } else if (current.isPresent()) { + return true; + } + return true; + } + protected boolean isWriteApplicable(final NodeModification modification, final Optional current) { Optional original = modification.getOriginal(); if (original.isPresent() && current.isPresent()) { @@ -174,6 +186,10 @@ public abstract class SchemaAwareApplyOperation implements ModificationApplyOper modification); return modification.storeSnapshot(Optional.of(applySubtreeChange(modification, currentMeta.get(), subtreeVersion))); + case MERGE: + if(currentMeta.isPresent()) { + return modification.storeSnapshot(Optional.of(applyMerge(modification,currentMeta.get(),subtreeVersion))); + } // Fallback to write is intentional - if node is not preexisting merge is same as write case WRITE: return modification.storeSnapshot(Optional.of(applyWrite(modification, currentMeta, subtreeVersion))); case UNMODIFIED: @@ -183,6 +199,9 @@ public abstract class SchemaAwareApplyOperation implements ModificationApplyOper } } + protected abstract StoreMetadataNode applyMerge(NodeModification modification, + StoreMetadataNode currentMeta, UnsignedLong subtreeVersion); + protected abstract StoreMetadataNode applyWrite(NodeModification modification, Optional currentMeta, UnsignedLong subtreeVersion); @@ -219,14 +238,16 @@ public abstract class SchemaAwareApplyOperation implements ModificationApplyOper + "is leaf type node. Subtree change is not allowed."); } + @Override + protected StoreMetadataNode applyMerge(final NodeModification modification, final StoreMetadataNode currentMeta, + final UnsignedLong subtreeVersion) { + return applyWrite(modification, Optional.of(currentMeta), subtreeVersion); + } + @Override protected StoreMetadataNode applyWrite(final NodeModification modification, final Optional currentMeta, final UnsignedLong subtreeVersion) { UnsignedLong nodeVersion = subtreeVersion; - if (currentMeta.isPresent()) { - nodeVersion = StoreUtils.increase(currentMeta.get().getNodeVersion()); - } - return StoreMetadataNode.builder().setNodeVersion(nodeVersion).setSubtreeVersion(subtreeVersion) .setData(modification.getWritenValue()).build(); } @@ -314,6 +335,13 @@ public abstract class SchemaAwareApplyOperation implements ModificationApplyOper } + @Override + protected StoreMetadataNode applyMerge(final NodeModification modification, final StoreMetadataNode currentMeta, + final UnsignedLong subtreeVersion) { + // For Node Containers - merge is same as subtree change - we only replace children. + return applySubtreeChange(modification, currentMeta, subtreeVersion); + } + @Override public StoreMetadataNode applySubtreeChange(final NodeModification modification, final StoreMetadataNode currentMeta, final UnsignedLong subtreeVersion) { @@ -569,7 +597,11 @@ public abstract class SchemaAwareApplyOperation implements ModificationApplyOper entryStrategy = Optional. of(new UnkeyedListItemModificationStrategy(schema)); } - + @Override + protected StoreMetadataNode applyMerge(final NodeModification modification, final StoreMetadataNode currentMeta, + final UnsignedLong subtreeVersion) { + return applyWrite(modification, Optional.of(currentMeta), subtreeVersion); + } @Override protected StoreMetadataNode applySubtreeChange(final NodeModification modification, diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ModificationType.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ModificationType.java index 199d90252e..b16e907120 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ModificationType.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ModificationType.java @@ -32,5 +32,12 @@ public enum ModificationType { * Tree node is to be deleted. * */ - DELETE + DELETE, + + /** + * + * Tree node is to be merged with existing one. + * + */ + MERGE } diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/NodeModification.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/NodeModification.java index 04fb3b7c6c..4f650c1711 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/NodeModification.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/NodeModification.java @@ -20,7 +20,6 @@ import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; import com.google.common.base.Optional; import com.google.common.base.Predicate; -import com.google.common.primitives.UnsignedLong; /** * Node Modification Node and Tree @@ -37,7 +36,9 @@ public class NodeModification implements StoreTreeNode, Identi public static final Predicate IS_TERMINAL_PREDICATE = new Predicate() { @Override public boolean apply(final NodeModification input) { - return input.getModificationType() == ModificationType.WRITE || input.getModificationType() == ModificationType.DELETE; + return input.getModificationType() == ModificationType.WRITE // + || input.getModificationType() == ModificationType.DELETE // + || input.getModificationType() == ModificationType.MERGE; } }; private final PathArgument identifier; @@ -48,7 +49,6 @@ public class NodeModification implements StoreTreeNode, Identi private NormalizedNode value; - private UnsignedLong subtreeVersion; private Optional snapshotCache; private final Map childModification; @@ -176,6 +176,14 @@ public class NodeModification implements StoreTreeNode, Identi this.value = value; } + public synchronized void merge(final NormalizedNode data) { + checkSealed(); + clearSnapshot(); + updateModificationType(ModificationType.MERGE); + // FIXME: Probably merge with previous value. + this.value = data; + } + @GuardedBy("this") private void checkSealed() { checkState(!sealed, "Node Modification is sealed. No further changes allowed."); diff --git a/opendaylight/md-sal/sal-dom-spi/src/main/java/org/opendaylight/controller/sal/core/spi/data/DOMStoreWriteTransaction.java b/opendaylight/md-sal/sal-dom-spi/src/main/java/org/opendaylight/controller/sal/core/spi/data/DOMStoreWriteTransaction.java index 6761bc1968..ddabbc6c03 100644 --- a/opendaylight/md-sal/sal-dom-spi/src/main/java/org/opendaylight/controller/sal/core/spi/data/DOMStoreWriteTransaction.java +++ b/opendaylight/md-sal/sal-dom-spi/src/main/java/org/opendaylight/controller/sal/core/spi/data/DOMStoreWriteTransaction.java @@ -33,6 +33,25 @@ public interface DOMStoreWriteTransaction extends DOMStoreTransaction { */ void write(InstanceIdentifier path, NormalizedNode data); + /** + * Store a provided data at specified path. This acts as a add / replace + * operation, which is to say that whole subtree will be replaced by + * specified path. + * + * If you need add or merge of current object with specified use + * {@link #merge(LogicalDatastoreType, Path, Object)} + * + * + * @param path + * @param data + * Data object to be written + * + * @throws IllegalStateException + * if the client code already sealed transaction and invoked + * {@link #ready()} + */ + void merge(InstanceIdentifier path, NormalizedNode data); + /** * * Deletes data and whole subtree located at provided path. -- 2.36.6