Force pruning during data migration 67/87067/2
authorRobert Varga <robert.varga@pantheon.tech>
Tue, 21 Jan 2020 13:31:48 +0000 (14:31 +0100)
committerRobert Varga <robert.varga@pantheon.tech>
Wed, 22 Jan 2020 10:47:57 +0000 (11:47 +0100)
Our current PruningDataTreeModification performs pruning only in
two cases:
1) the root is being written (i.e. Snapshot recovery)
2) DataTree reports a validation issue

This is not sufficient for uint-type migration, as we have to
subject all writes and merges to pruning/translation irrespective
of where they occur.

Split up PruningDataTreeModification into two implementations,
Proactive and Reactive and use them as appropriate based on migration
requirements.

JIRA: CONTROLLER-1923
Change-Id: I4184c56380b7f52a8d7af6f17346f9c5edd31b28
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardDataTree.java
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/utils/PruningDataTreeModification.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardDataTreeTest.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/utils/PruningDataTreeModificationTest.java

index a097b68..5a59b0d 100644 (file)
@@ -311,32 +311,27 @@ public class ShardDataTree extends ShardDataTreeTransactionParent {
      */
     void applyRecoverySnapshot(final @NonNull ShardSnapshotState snapshot) throws DataValidationFailedException {
         // TODO: we should be able to reuse the pruner, provided we are not reentrant
-        ReusableNormalizedNodePruner pruner = ReusableNormalizedNodePruner.forDataSchemaContext(dataSchemaContext);
+        final ReusableNormalizedNodePruner pruner = ReusableNormalizedNodePruner.forDataSchemaContext(
+            dataSchemaContext);
         if (snapshot.needsMigration()) {
-            pruner = pruner.withUintAdaption();
+            final ReusableNormalizedNodePruner uintPruner = pruner.withUintAdaption();
+            applySnapshot(snapshot.getSnapshot(),
+                delegate -> new PruningDataTreeModification.Proactive(delegate, dataTree, uintPruner));
+        } else {
+            applySnapshot(snapshot.getSnapshot(),
+                delegate -> new PruningDataTreeModification.Reactive(delegate, dataTree, pruner));
         }
-
-        // For lambda below
-        final ReusableNormalizedNodePruner finalPruner = pruner;
-        applySnapshot(snapshot.getSnapshot(),
-            delegate -> new PruningDataTreeModification(delegate, dataTree, finalPruner));
     }
 
     @SuppressWarnings("checkstyle:IllegalCatch")
     private void applyRecoveryCandidate(final CommitTransactionPayload payload) throws IOException {
         final Entry<TransactionIdentifier, DataTreeCandidateWithVersion> entry = payload.getCandidate();
         final DataTreeModification unwrapped = dataTree.takeSnapshot().newModification();
+        final PruningDataTreeModification mod = createPruningModification(unwrapped,
+            NormalizedNodeStreamVersion.MAGNESIUM.compareTo(entry.getValue().getVersion()) > 0);
 
-        // TODO: we should be able to reuse the pruner, provided we are not reentrant
-        ReusableNormalizedNodePruner pruner = ReusableNormalizedNodePruner.forDataSchemaContext(dataSchemaContext);
-        if (NormalizedNodeStreamVersion.MAGNESIUM.compareTo(entry.getValue().getVersion()) > 0) {
-            pruner = pruner.withUintAdaption();
-        }
-
-        final PruningDataTreeModification mod = new PruningDataTreeModification(unwrapped, dataTree, pruner);
         DataTreeCandidates.applyToModification(mod, entry.getValue().getCandidate());
         mod.ready();
-
         LOG.trace("{}: Applying recovery modification {}", logContext, unwrapped);
 
         try {
@@ -354,6 +349,15 @@ public class ShardDataTree extends ShardDataTreeTransactionParent {
         allMetadataCommittedTransaction(entry.getKey());
     }
 
+    private PruningDataTreeModification createPruningModification(final DataTreeModification unwrapped,
+            final boolean uintAdapting) {
+        // TODO: we should be able to reuse the pruner, provided we are not reentrant
+        final ReusableNormalizedNodePruner pruner = ReusableNormalizedNodePruner.forDataSchemaContext(
+            dataSchemaContext);
+        return uintAdapting ? new PruningDataTreeModification.Proactive(unwrapped, dataTree, pruner.withUintAdaption())
+                : new PruningDataTreeModification.Reactive(unwrapped, dataTree, pruner);
+    }
+
     /**
      * Apply a payload coming from recovery. This method does not assume the SchemaContexts match and performs data
      * pruning in an attempt to adjust the state to our current SchemaContext.
index 8b97132..8ce7119 100644 (file)
@@ -31,7 +31,82 @@ import org.slf4j.LoggerFactory;
  * The PruningDataTreeModification first removes all entries from the data which do not belong in the schemaContext
  * before delegating it to the actual DataTreeModification.
  */
-public class PruningDataTreeModification extends ForwardingObject implements DataTreeModification {
+public abstract class PruningDataTreeModification extends ForwardingObject implements DataTreeModification {
+    /**
+     * A PruningDataTreeModification which always performs pruning before attempting an operation. This sacrifices
+     * performance to ensure all data has passed through the pruner -- such that data adaptations are performed.
+     */
+    public static final class Proactive extends PruningDataTreeModification {
+        public Proactive(final DataTreeModification delegate, final DataTree dataTree,
+                final ReusableNormalizedNodePruner pruner) {
+            super(delegate, dataTree, pruner);
+        }
+
+        @Override
+        public void merge(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
+            pruneAndMergeNode(path, data);
+        }
+
+        @Override
+        public void write(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
+            pruneAndWriteNode(path, data);
+        }
+
+        @Override
+        PruningDataTreeModification createNew(final DataTreeModification delegate, final DataTree dataTree,
+                final ReusableNormalizedNodePruner pruner) {
+            return new Proactive(delegate, dataTree, pruner);
+        }
+    }
+
+    /**
+     * A PruningDataTreeModification which performs pruning only when an operation results in an
+     * {@link SchemaValidationFailedException}. This offers superior performance in the normal case of not needing
+     * pruning.
+     */
+    public static final class Reactive extends PruningDataTreeModification {
+        public Reactive(final DataTreeModification delegate, final DataTree dataTree,
+                final ReusableNormalizedNodePruner pruner) {
+            super(delegate, dataTree, pruner);
+        }
+
+        @Override
+        public void merge(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
+            if (path.isEmpty()) {
+                pruneAndMergeNode(path, data);
+                return;
+            }
+
+            try {
+                delegate().merge(path, data);
+            } catch (SchemaValidationFailedException e) {
+                LOG.warn("Node at path {} was pruned during merge due to validation error: {}", path, e.getMessage());
+                pruneAndMergeNode(path, data);
+            }
+        }
+
+        @Override
+        public void write(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
+            if (path.isEmpty()) {
+                pruneAndWriteNode(path, data);
+                return;
+            }
+
+            try {
+                delegate().write(path, data);
+            } catch (SchemaValidationFailedException e) {
+                LOG.warn("Node at path : {} was pruned during write due to validation error: {}", path, e.getMessage());
+                pruneAndWriteNode(path, data);
+            }
+        }
+
+        @Override
+        PruningDataTreeModification createNew(final DataTreeModification delegate, final DataTree dataTree,
+                final ReusableNormalizedNodePruner pruner) {
+            return new Reactive(delegate, dataTree, pruner);
+        }
+    }
+
     private static final Logger LOG = LoggerFactory.getLogger(PruningDataTreeModification.class);
 
     private final ReusableNormalizedNodePruner pruner;
@@ -39,7 +114,7 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat
 
     private DataTreeModification delegate;
 
-    public PruningDataTreeModification(final DataTreeModification delegate, final DataTree dataTree,
+    PruningDataTreeModification(final DataTreeModification delegate, final DataTree dataTree,
             final ReusableNormalizedNodePruner pruner) {
         this.delegate = requireNonNull(delegate);
         this.dataTree = requireNonNull(dataTree);
@@ -47,17 +122,17 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat
     }
 
     @Override
-    protected DataTreeModification delegate() {
+    protected final DataTreeModification delegate() {
         return delegate;
     }
 
     @Override
-    public SchemaContext getSchemaContext() {
+    public final SchemaContext getSchemaContext() {
         return delegate.getSchemaContext();
     }
 
     @Override
-    public void delete(final YangInstanceIdentifier path) {
+    public final void delete(final YangInstanceIdentifier path) {
         try {
             delegate.delete(path);
         } catch (SchemaValidationFailedException e) {
@@ -65,44 +140,14 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat
         }
     }
 
-    @Override
-    public void merge(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
-        if (path.isEmpty()) {
-            pruneAndMergeNode(path, data);
-            return;
-        }
-
-        try {
-            delegate.merge(path, data);
-        } catch (SchemaValidationFailedException e) {
-            LOG.warn("Node at path {} was pruned during merge due to validation error: {}", path, e.getMessage());
-            pruneAndMergeNode(path, data);
-        }
-    }
-
-    private void pruneAndMergeNode(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
+    final void pruneAndMergeNode(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
         final NormalizedNode<?, ?> pruned = pruneNormalizedNode(path, data);
         if (pruned != null) {
             delegate.merge(path, pruned);
         }
     }
 
-    @Override
-    public void write(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
-        if (path.isEmpty()) {
-            pruneAndWriteNode(path, data);
-            return;
-        }
-
-        try {
-            delegate.write(path, data);
-        } catch (SchemaValidationFailedException e) {
-            LOG.warn("Node at path : {} was pruned during write due to validation error: {}", path, e.getMessage());
-            pruneAndWriteNode(path, data);
-        }
-    }
-
-    private void pruneAndWriteNode(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
+    final void pruneAndWriteNode(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
         final NormalizedNode<?, ?> pruned = pruneNormalizedNode(path, data);
         if (pruned != null) {
             delegate.write(path, pruned);
@@ -110,7 +155,7 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat
     }
 
     @Override
-    public void ready() {
+    public final void ready() {
         try {
             delegate.ready();
         } catch (SchemaValidationFailedException e) {
@@ -123,22 +168,23 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat
     }
 
     @Override
-    public void applyToCursor(final DataTreeModificationCursor dataTreeModificationCursor) {
+    public final void applyToCursor(final DataTreeModificationCursor dataTreeModificationCursor) {
         delegate.applyToCursor(dataTreeModificationCursor);
     }
 
     @Override
-    public Optional<NormalizedNode<?, ?>> readNode(final YangInstanceIdentifier yangInstanceIdentifier) {
+    public final Optional<NormalizedNode<?, ?>> readNode(final YangInstanceIdentifier yangInstanceIdentifier) {
         return delegate.readNode(yangInstanceIdentifier);
     }
 
     @Override
-    public DataTreeModification newModification() {
-        return new PruningDataTreeModification(delegate.newModification(), dataTree, pruner.duplicate());
+    public final DataTreeModification newModification() {
+        return createNew(delegate.newModification(), dataTree, pruner.duplicate());
     }
 
     @VisibleForTesting
-    NormalizedNode<?, ?> pruneNormalizedNode(final YangInstanceIdentifier path, final NormalizedNode<?, ?> input) {
+    final NormalizedNode<?, ?> pruneNormalizedNode(final YangInstanceIdentifier path,
+            final NormalizedNode<?, ?> input) {
         pruner.initializeForPath(path);
         try {
             NormalizedNodeWriter.forStreamWriter(pruner).write(input);
@@ -150,7 +196,10 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat
         return pruner.getResult().orElse(null);
     }
 
-    private static class PruningDataTreeModificationCursor extends AbstractDataTreeModificationCursor {
+    abstract PruningDataTreeModification createNew(DataTreeModification delegate, DataTree dataTree,
+            ReusableNormalizedNodePruner pruner);
+
+    private static final class PruningDataTreeModificationCursor extends AbstractDataTreeModificationCursor {
         private final DataTreeModification toModification;
         private final PruningDataTreeModification pruningModification;
 
index 43dd465..abaefcb 100644 (file)
@@ -65,8 +65,10 @@ import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdent
 import org.opendaylight.yangtools.yang.data.api.schema.ContainerNode;
 import org.opendaylight.yangtools.yang.data.api.schema.MapEntryNode;
 import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
+import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTree;
 import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeCandidate;
 import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeCandidates;
+import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeConfiguration;
 import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeModification;
 import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeSnapshot;
 import org.opendaylight.yangtools.yang.data.api.schema.tree.DataValidationFailedException;
@@ -74,6 +76,7 @@ import org.opendaylight.yangtools.yang.data.api.schema.tree.ModificationType;
 import org.opendaylight.yangtools.yang.data.api.schema.tree.TreeType;
 import org.opendaylight.yangtools.yang.data.impl.schema.Builders;
 import org.opendaylight.yangtools.yang.data.impl.schema.ImmutableNodes;
+import org.opendaylight.yangtools.yang.data.impl.schema.tree.InMemoryDataTreeFactory;
 import org.opendaylight.yangtools.yang.model.api.SchemaContext;
 
 public class ShardDataTreeTest extends AbstractTest {
@@ -504,19 +507,74 @@ public class ShardDataTreeTest extends AbstractTest {
         assertCarsUint64();
     }
 
+    @Test
+    public void testUintReplay() throws DataValidationFailedException, IOException {
+        // Commit two writes and one merge, saving the data tree candidate for each.
+        //        write(foo=1)
+        //        write(foo=2)
+        //        merge(foo=3)
+        final DataTree dataTree = new InMemoryDataTreeFactory().create(DataTreeConfiguration.DEFAULT_OPERATIONAL,
+            fullSchema);
+        DataTreeModification mod = dataTree.takeSnapshot().newModification();
+        mod.write(CarsModel.BASE_PATH, Builders.containerBuilder()
+                .withNodeIdentifier(new NodeIdentifier(CarsModel.BASE_QNAME))
+                .withChild(Builders.mapBuilder()
+                    .withNodeIdentifier(new NodeIdentifier(CarsModel.CAR_QNAME))
+                    .withChild(createCar("one", BigInteger.ONE))
+                    .build())
+                .build());
+        mod.ready();
+        dataTree.validate(mod);
+        final DataTreeCandidate first = dataTree.prepare(mod);
+        dataTree.commit(first);
+
+        mod = dataTree.takeSnapshot().newModification();
+        mod.write(CarsModel.newCarPath("two"), createCar("two", BigInteger.TWO));
+        mod.ready();
+        dataTree.validate(mod);
+        final DataTreeCandidate second = dataTree.prepare(mod);
+        dataTree.commit(second);
+
+        mod = dataTree.takeSnapshot().newModification();
+        mod.merge(CarsModel.CAR_LIST_PATH, Builders.mapBuilder()
+            .withNodeIdentifier(new NodeIdentifier(CarsModel.CAR_QNAME))
+            .withChild(createCar("three", BigInteger.TEN))
+            .build());
+        mod.ready();
+        dataTree.validate(mod);
+        final DataTreeCandidate third = dataTree.prepare(mod);
+        dataTree.commit(third);
+
+        // Apply first candidate as a snapshot
+        shardDataTree.applyRecoverySnapshot(
+            new ShardSnapshotState(new MetadataShardDataTreeSnapshot(first.getRootNode().getDataAfter().get()), true));
+        // Apply the other two snapshots as transactions
+        shardDataTree.applyRecoveryPayload(CommitTransactionPayload.create(nextTransactionId(), second,
+            PayloadVersion.SODIUM_SR1));
+        shardDataTree.applyRecoveryPayload(CommitTransactionPayload.create(nextTransactionId(), third,
+            PayloadVersion.SODIUM_SR1));
+
+        // Verify uint translation
+        final DataTreeSnapshot snapshot = shardDataTree.newReadOnlyTransaction(nextTransactionId()).getSnapshot();
+        final NormalizedNode<?, ?> cars = snapshot.readNode(CarsModel.CAR_LIST_PATH).get();
+
+        assertEquals(Builders.mapBuilder()
+            .withNodeIdentifier(new NodeIdentifier(CarsModel.CAR_QNAME))
+            // Note: Uint64
+            .withChild(createCar("one", Uint64.ONE))
+            .withChild(createCar("two", Uint64.TWO))
+            .withChild(createCar("three", Uint64.TEN))
+            .build(), cars);
+    }
+
     private void assertCarsUint64() {
         final DataTreeSnapshot snapshot = shardDataTree.newReadOnlyTransaction(nextTransactionId()).getSnapshot();
         final NormalizedNode<?, ?> cars = snapshot.readNode(CarsModel.CAR_LIST_PATH).get();
 
         assertEquals(Builders.mapBuilder()
             .withNodeIdentifier(new NodeIdentifier(CarsModel.CAR_QNAME))
-            .withChild(Builders.mapEntryBuilder()
-                .withNodeIdentifier(NodeIdentifierWithPredicates.of(CarsModel.CAR_QNAME,
-                    CarsModel.CAR_NAME_QNAME, "foo"))
-                .withChild(ImmutableNodes.leafNode(CarsModel.CAR_NAME_QNAME, "foo"))
-                // Note: Uint64
-                .withChild(ImmutableNodes.leafNode(CarsModel.CAR_PRICE_QNAME, Uint64.ONE))
-                .build())
+            // Note: Uint64
+            .withChild(createCar("foo", Uint64.ONE))
             .build(), cars);
     }
 
@@ -527,18 +585,22 @@ public class ShardDataTreeTest extends AbstractTest {
                     .withNodeIdentifier(new NodeIdentifier(CarsModel.CARS_QNAME))
                     .withChild(Builders.mapBuilder()
                         .withNodeIdentifier(new NodeIdentifier(CarsModel.CAR_QNAME))
-                        .withChild(Builders.mapEntryBuilder()
-                            .withNodeIdentifier(NodeIdentifierWithPredicates.of(CarsModel.CAR_QNAME,
-                                CarsModel.CAR_NAME_QNAME, "foo"))
-                            .withChild(ImmutableNodes.leafNode(CarsModel.CAR_NAME_QNAME, "foo"))
-                            // Note: old BigInteger
-                            .withChild(ImmutableNodes.leafNode(CarsModel.CAR_PRICE_QNAME, BigInteger.ONE))
-                            .build())
+                        // Note: BigInteger
+                        .withChild(createCar("foo", BigInteger.ONE))
                         .build())
                     .build())
                 .build();
     }
 
+    private static MapEntryNode createCar(final String name, final Object value) {
+        return Builders.mapEntryBuilder()
+                .withNodeIdentifier(NodeIdentifierWithPredicates.of(CarsModel.CAR_QNAME,CarsModel.CAR_NAME_QNAME, name))
+                .withChild(ImmutableNodes.leafNode(CarsModel.CAR_NAME_QNAME, name))
+                // Note: old BigInteger
+                .withChild(ImmutableNodes.leafNode(CarsModel.CAR_PRICE_QNAME, value))
+                .build();
+    }
+
     private ShardDataTreeCohort newShardDataTreeCohort(final DataTreeOperation operation) {
         final ReadWriteShardDataTreeTransaction transaction =
                 shardDataTree.newReadWriteTransaction(nextTransactionId());
index 9118d8f..6ec01cf 100644 (file)
@@ -98,7 +98,7 @@ public class PruningDataTreeModificationTest {
             }
         });
 
-        pruningDataTreeModification = new PruningDataTreeModification(proxyModification, dataTree,
+        pruningDataTreeModification = new PruningDataTreeModification.Reactive(proxyModification, dataTree,
             // Cannot reuse with parallel tests
             ReusableNormalizedNodePruner.forDataSchemaContext(CONTEXT_TREE));
     }

©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.