From 7b7186bca2b89bd692de06ac2785d5d850c0b973 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Wed, 5 Apr 2017 18:36:48 +0200 Subject: [PATCH 1/1] 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 --- .../dds/FailedDataTreeModification.java | 70 ---------------- .../dds/LocalReadWriteProxyTransaction.java | 81 +++++++++++++++---- .../dds/FailedDataTreeModificationTest.java | 77 ------------------ 3 files changed, 67 insertions(+), 161 deletions(-) delete mode 100644 opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/databroker/actors/dds/FailedDataTreeModification.java delete mode 100644 opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/databroker/actors/dds/FailedDataTreeModificationTest.java 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 index 7c8bd9211e..0000000000 --- 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 supplier; - - FailedDataTreeModification(final Supplier supplier) { - this.supplier = Preconditions.checkNotNull(supplier); - } - - @Override - public Optional> 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 18d596f996..e2e93de98c 100644 --- 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 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 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. + * + *

+ * 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> 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> 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> 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 index da7f9dc7a2..0000000000 --- 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); - } -} -- 2.36.6