BUG-5280: add the concept of a recorded failure

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>

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
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/FailedDataTreeModification.java
+++ /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();
-    }
-}

diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/LocalReadWriteProxyTransaction.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/LocalReadWriteProxyTransaction.java
index 18d596f9964207a3a7405b28afb74c6636ec5ca8..e2e93de98ccf671df3dab6d4bd4a5148d5abcb53 100644 (file)
--- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/LocalReadWriteProxyTransaction.java
+++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/LocalReadWriteProxyTransaction.java
@@ -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 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;
 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);
 
 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;
 
     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);
     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() {
 
     @Override
     CursorAwareDataTreeSnapshot readOnlyView() {

-        return modification;
+        return getModification();

     }
 
     @Override
     }
 
     @Override

+    @SuppressWarnings("checkstyle:IllegalCatch")

     void doDelete(final YangInstanceIdentifier path) {
     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
     }
 
     @Override

+    @SuppressWarnings("checkstyle:IllegalCatch")

     void doMerge(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
     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
     }
 
     @Override

+    @SuppressWarnings("checkstyle:IllegalCatch")

     void doWrite(final YangInstanceIdentifier path, final NormalizedNode<?, ?> data) {
     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() {
     }
 
     private RuntimeException abortedException() {


@@ -100,16 +140,19 @@ final class LocalReadWriteProxyTransaction extends LocalProxyTransaction {
 
     @Override
     CommitLocalTransactionRequest commitRequest(final boolean coordinated) {
 
     @Override
     CommitLocalTransactionRequest commitRequest(final boolean coordinated) {

+        final CursorAwareDataTreeModification mod = getModification();

         final CommitLocalTransactionRequest ret = new CommitLocalTransactionRequest(getIdentifier(), nextSequence(),
         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() {
         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
     }
 
     @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) {
             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) {
             } else if (mod instanceof TransactionMerge) {

-                modification.merge(mod.getPath(), ((TransactionMerge)mod).getData());
+                getModification().merge(mod.getPath(), ((TransactionMerge)mod).getData());

             } else if (mod instanceof TransactionDelete) {
             } else if (mod instanceof TransactionDelete) {

-                modification.delete(mod.getPath());
+                getModification().delete(mod.getPath());

             } else {
                 throw new IllegalArgumentException("Unsupported modification " + mod);
             }
             } 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);
     @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.
     }
 
     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);
         }
 
             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
--- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/actors/dds/FailedDataTreeModificationTest.java
+++ /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);
-    }
-}