BUG-5280: add the concept of a recorded failure 71/54371/4
authorRobert Varga <robert.varga@pantheon.tech>
Wed, 5 Apr 2017 16:36:48 +0000 (18:36 +0200)
committerRobert Varga <robert.varga@pantheon.tech>
Thu, 6 Apr 2017 17:43:03 +0000 (19:43 +0200)
This patch reworks LocalReadWriteProxyTransaction to be defensive
of its internal modification and introduces the concept of a delayed
recorded error (currently unused).

The defensiveness checks allow us to get rid of FailedDataTreeModification,
as we do not give out our modification at all in the codepaths which
would leak this implementation.

Change-Id: I5f91218ac308f7450a3b59252d44f953be54626c
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/FailedDataTreeModification.java [deleted file]
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/LocalReadWriteProxyTransaction.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/actors/dds/FailedDataTreeModificationTest.java [deleted file]

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/FailedDataTreeModification.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/FailedDataTreeModification.java
deleted file mode 100644 (file)
index 7c8bd92..0000000
+++ /dev/null
@@ -1,70 +0,0 @@
-/*
- * Copyright (c) 2016 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.controller.cluster.databroker.actors.dds;
-
-import com.google.common.base.Optional;
-import com.google.common.base.Preconditions;
-import java.util.function.Supplier;
-import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
-import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
-import org.opendaylight.yangtools.yang.data.api.schema.tree.CursorAwareDataTreeModification;
-import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeModificationCursor;
-
-/**
- * An implementation of DataTreeModification which throws a specified {@link RuntimeException} when any of its methods
- * are invoked.
- *
- * @author Robert Varga
- */
-final class FailedDataTreeModification implements CursorAwareDataTreeModification {
-    private final Supplier<? extends RuntimeException> supplier;
-
-    FailedDataTreeModification(final Supplier<? extends RuntimeException> supplier) {
-        this.supplier = Preconditions.checkNotNull(supplier);
-    }
-
-    @Override
-    public Optional<NormalizedNode<?, ?>> readNode(final YangInstanceIdentifier path) {
-        throw supplier.get();
-    }
-
-    @Override
-    public CursorAwareDataTreeModification newModification() {
-        throw supplier.get();
-    }
-
-    @Override
-    public void delete(final YangInstanceIdentifier path) {
-        throw supplier.get();
-    }
-
-    @Override
-    public void merge(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
-        throw supplier.get();
-    }
-
-    @Override
-    public void write(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
-        throw supplier.get();
-    }
-
-    @Override
-    public void ready() {
-        throw supplier.get();
-    }
-
-    @Override
-    public void applyToCursor(final DataTreeModificationCursor cursor) {
-        throw supplier.get();
-    }
-
-    @Override
-    public DataTreeModificationCursor createCursor(final YangInstanceIdentifier path) {
-        throw supplier.get();
-    }
-}
index 18d596f..e2e93de 100644 (file)
@@ -10,6 +10,7 @@ package org.opendaylight.controller.cluster.databroker.actors.dds;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Verify;
 import java.util.function.Consumer;
+import java.util.function.Supplier;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.NotThreadSafe;
 import org.opendaylight.controller.cluster.access.commands.CommitLocalTransactionRequest;
@@ -56,9 +57,27 @@ import org.slf4j.LoggerFactory;
 final class LocalReadWriteProxyTransaction extends LocalProxyTransaction {
     private static final Logger LOG = LoggerFactory.getLogger(LocalReadWriteProxyTransaction.class);
 
-    private CursorAwareDataTreeModification modification;
+    /**
+     * This field needs to be accessed via {@link #getModification()}, which performs state checking to ensure
+     * the modification can actually be accessed.
+     */
+    private final CursorAwareDataTreeModification modification;
+
+    private Supplier<? extends RuntimeException> closedException;
+
     private CursorAwareDataTreeModification sealedModification;
 
+    /**
+     * Recorded failure from previous operations. Normally we would want to propagate the error directly to the
+     * offending call site, but that exposes inconsistency in behavior during initial connection, when we go through
+     * {@link RemoteProxyTransaction}, which detects this sort of issues at canCommit/directCommit time on the backend.
+     *
+     * <p>
+     * We therefore do not report incurred exceptions directly, but report them once the user attempts to commit
+     * this transaction.
+     */
+    private Exception recordedFailure;
+
     LocalReadWriteProxyTransaction(final ProxyHistory parent, final TransactionIdentifier identifier,
         final DataTreeSnapshot snapshot) {
         super(parent, identifier);
@@ -72,22 +91,43 @@ final class LocalReadWriteProxyTransaction extends LocalProxyTransaction {
 
     @Override
     CursorAwareDataTreeSnapshot readOnlyView() {
-        return modification;
+        return getModification();
     }
 
     @Override
+    @SuppressWarnings("checkstyle:IllegalCatch")
     void doDelete(final YangInstanceIdentifier path) {
-        modification.delete(path);
+        final CursorAwareDataTreeModification mod = getModification();
+        if (recordedFailure != null) {
+            LOG.debug("Transaction {} recorded failure, ignoring delete of {}", getIdentifier(), path);
+            return;
+        }
+
+        mod.delete(path);
     }
 
     @Override
+    @SuppressWarnings("checkstyle:IllegalCatch")
     void doMerge(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
-        modification.merge(path, data);
+        final CursorAwareDataTreeModification mod = getModification();
+        if (recordedFailure != null) {
+            LOG.debug("Transaction {} recorded failure, ignoring merge to {}", getIdentifier(), path);
+            return;
+        }
+
+        mod.merge(path, data);
     }
 
     @Override
+    @SuppressWarnings("checkstyle:IllegalCatch")
     void doWrite(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
-        modification.write(path, data);
+        final CursorAwareDataTreeModification mod = getModification();
+        if (recordedFailure != null) {
+            LOG.debug("Transaction {} recorded failure, ignoring write to {}", getIdentifier(), path);
+            return;
+        }
+
+        mod.write(path, data);
     }
 
     private RuntimeException abortedException() {
@@ -100,16 +140,19 @@ final class LocalReadWriteProxyTransaction extends LocalProxyTransaction {
 
     @Override
     CommitLocalTransactionRequest commitRequest(final boolean coordinated) {
+        final CursorAwareDataTreeModification mod = getModification();
         final CommitLocalTransactionRequest ret = new CommitLocalTransactionRequest(getIdentifier(), nextSequence(),
-            localActor(), modification, coordinated);
-        modification = new FailedDataTreeModification(this::submittedException);
+            localActor(), mod, coordinated);
+        closedException = this::submittedException;
         return ret;
     }
 
     @Override
     void doSeal() {
-        modification.ready();
-        sealedModification = modification;
+        Preconditions.checkState(sealedModification == null, "Transaction %s is already sealed", getIdentifier());
+        final CursorAwareDataTreeModification mod = getModification();
+        mod.ready();
+        sealedModification = mod;
     }
 
     @Override
@@ -142,11 +185,11 @@ final class LocalReadWriteProxyTransaction extends LocalProxyTransaction {
             final @Nullable Consumer<Response<?, ?>> callback) {
         for (final TransactionModification mod : request.getModifications()) {
             if (mod instanceof TransactionWrite) {
-                modification.write(mod.getPath(), ((TransactionWrite)mod).getData());
+                getModification().write(mod.getPath(), ((TransactionWrite)mod).getData());
             } else if (mod instanceof TransactionMerge) {
-                modification.merge(mod.getPath(), ((TransactionMerge)mod).getData());
+                getModification().merge(mod.getPath(), ((TransactionMerge)mod).getData());
             } else if (mod instanceof TransactionDelete) {
-                modification.delete(mod.getPath());
+                getModification().delete(mod.getPath());
             } else {
                 throw new IllegalArgumentException("Unsupported modification " + mod);
             }
@@ -207,12 +250,22 @@ final class LocalReadWriteProxyTransaction extends LocalProxyTransaction {
     @Override
     void sendAbort(final TransactionRequest<?> request, final Consumer<Response<?, ?>> callback) {
         super.sendAbort(request, callback);
-        modification = new FailedDataTreeModification(this::abortedException);
+        closedException = this::abortedException;
+    }
+
+    private CursorAwareDataTreeModification getModification() {
+        if (closedException != null) {
+            throw closedException.get();
+        }
+
+        return modification;
     }
 
     private void sendCommit(final CommitLocalTransactionRequest request, final Consumer<Response<?, ?>> callback) {
         // Rebase old modification on new data tree.
-        try (DataTreeModificationCursor cursor = modification.createCursor(YangInstanceIdentifier.EMPTY)) {
+        final CursorAwareDataTreeModification mod = getModification();
+
+        try (DataTreeModificationCursor cursor = mod.createCursor(YangInstanceIdentifier.EMPTY)) {
             request.getModification().applyToCursor(cursor);
         }
 
diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/actors/dds/FailedDataTreeModificationTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/actors/dds/FailedDataTreeModificationTest.java
deleted file mode 100644 (file)
index da7f9dc..0000000
+++ /dev/null
@@ -1,77 +0,0 @@
-/*
- * Copyright (c) 2017 Pantheon Technologies s.r.o. 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.controller.cluster.databroker.actors.dds;
-
-import static org.mockito.MockitoAnnotations.initMocks;
-
-import java.util.function.Supplier;
-import org.junit.Before;
-import org.junit.Test;
-import org.mockito.Mock;
-import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
-import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
-import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeModificationCursor;
-
-public class FailedDataTreeModificationTest {
-
-    private FailedDataTreeModification failedDataTreeModification;
-
-    @Mock
-    private YangInstanceIdentifier instanceIdentifier;
-    @Mock
-    private NormalizedNode normalizedNode;
-    @Mock
-    private DataTreeModificationCursor dataTreeModificationCursor;
-
-    @Before
-    public void setUp() throws Exception {
-        initMocks(this);
-        final Supplier supplier = () -> new RuntimeException("Operation failed.");
-        failedDataTreeModification = new FailedDataTreeModification(supplier);
-    }
-
-    @Test(expected = RuntimeException.class)
-    public void testReadNode() throws Exception {
-        failedDataTreeModification.readNode(instanceIdentifier);
-    }
-
-    @Test(expected = RuntimeException.class)
-    public void testNewModification() throws Exception {
-        failedDataTreeModification.newModification();
-    }
-
-    @Test(expected = RuntimeException.class)
-    public void testDelete() throws Exception {
-        failedDataTreeModification.delete(instanceIdentifier);
-    }
-
-    @Test(expected = RuntimeException.class)
-    public void testMerge() throws Exception {
-        failedDataTreeModification.merge(instanceIdentifier, normalizedNode);
-    }
-
-    @Test(expected = RuntimeException.class)
-    public void testWrite() throws Exception {
-        failedDataTreeModification.write(instanceIdentifier, normalizedNode);
-    }
-
-    @Test(expected = RuntimeException.class)
-    public void testReady() throws Exception {
-        failedDataTreeModification.ready();
-    }
-
-    @Test(expected = RuntimeException.class)
-    public void testApplyToCursor() throws Exception {
-        failedDataTreeModification.applyToCursor(dataTreeModificationCursor);
-    }
-
-    @Test(expected = RuntimeException.class)
-    public void testCreateCursor() throws Exception {
-        failedDataTreeModification.createCursor(instanceIdentifier);
-    }
-}

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