From bb090e1254eacc95d7bd1dd45f6311967f81af86 Mon Sep 17 00:00:00 2001 From: Jan Srnicek Date: Mon, 24 Oct 2016 13:02:59 +0200 Subject: HONEYCOMB-255 - Cutting identifiers to prevent failing of reverts Mapping allready processes changes for reverting by InstanceIdentifier instead of using KeyedInstanceIdentifier(to prevent failing to identify handleable nodes) Modified logging to prevent double/triple logging of detailed cause of failed bulk update Reusing WriteContext for revert(removed try with resource to prevent closing of write context before revert) Change-Id: Ie939ebe443629f9cdad5b5b449aa8c5dac40ea67 Signed-off-by: Jan Srnicek Signed-off-by: Maros Marsalek --- .../data/impl/ModifiableDataTreeDelegator.java | 31 +++++++++++++++++++--- .../fd/honeycomb/data/impl/WriteTransaction.java | 2 +- .../data/impl/ModifiableDataTreeDelegatorTest.java | 16 ++++++----- 3 files changed, 38 insertions(+), 11 deletions(-) (limited to 'infra/data-impl/src') diff --git a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegator.java b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegator.java index 213208064..3de9131c0 100644 --- a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegator.java +++ b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegator.java @@ -26,6 +26,7 @@ import com.google.common.collect.Multimap; import com.google.common.util.concurrent.CheckedFuture; import io.fd.honeycomb.data.DataModification; import io.fd.honeycomb.data.ReadableDataManager; +import io.fd.honeycomb.translate.MappingContext; import io.fd.honeycomb.translate.TranslationException; import io.fd.honeycomb.translate.util.RWUtils; import io.fd.honeycomb.translate.util.TransactionMappingContext; @@ -126,7 +127,8 @@ public final class ModifiableDataTreeDelegator extends ModifiableDataTreeManager final WriterRegistry.DataObjectUpdates baUpdates = toBindingAware(modificationDiff.getUpdates()); LOG.debug("ConfigDataTree.modify() extracted updates={}", baUpdates); - try (final WriteContext ctx = getTransactionWriteContext()) { + WriteContext ctx = getTransactionWriteContext(); + try { writerRegistry.update(baUpdates, ctx); final CheckedFuture contextUpdateResult = @@ -139,15 +141,18 @@ public final class ModifiableDataTreeDelegator extends ModifiableDataTreeManager LOG.info("Trying to revert successful changes for current transaction"); try { - e.revertChanges(); + // attempt revert with fresh context, to allow write logic used context-binded data + e.revertChanges(getRevertTransactionContext(ctx.getMappingContext())); LOG.info("Changes successfully reverted"); } catch (WriterRegistry.Reverter.RevertFailedException revertFailedException) { // fail with failed revert LOG.error("Failed to revert successful changes", revertFailedException); throw revertFailedException; } - - throw e; // fail with success revert + // fail with success revert + // not passing the cause,its logged above and it would be logged after transaction + // ended again(prevent double logging of same error + throw new WriterRegistry.Reverter.RevertSuccessException(e.getFailedIds()); } catch (TransactionCommitFailedException e) { // TODO HONEYCOMB-162 revert should probably occur when context is not written successfully final String msg = "Error while updating mapping context data"; @@ -156,9 +161,27 @@ public final class ModifiableDataTreeDelegator extends ModifiableDataTreeManager } catch (TranslationException e) { LOG.error("Error while processing data change (updates={})", baUpdates, e); throw e; + } finally { + // Using finally instead of try-with-resources in order to leave ctx open for BulkUpdateException catch + // block. The context is needed there, but try-with-resources closes the resource before handling ex. + LOG.debug("Closing write context {}", ctx); + ctx.close(); } } + /** + * Creates inverted transaction context for reverting of proceeded changes. + * Invert before/after transaction and reuse affected mapping context created by previous updates + * to access all data created by previous updates + * */ + private TransactionWriteContext getRevertTransactionContext(final MappingContext affectedMappingContext){ + // Before Tx == after partial update + final DOMDataReadOnlyTransaction beforeTx = ReadOnlyTransaction.create(this, EMPTY_OPERATIONAL); + // After Tx == before partial update + final DOMDataReadOnlyTransaction afterTx = ReadOnlyTransaction.create(untouchedModification, EMPTY_OPERATIONAL); + return new TransactionWriteContext(serializer, beforeTx, afterTx, affectedMappingContext); + } + private TransactionWriteContext getTransactionWriteContext() { // Before Tx must use modification final DOMDataReadOnlyTransaction beforeTx = ReadOnlyTransaction.create(untouchedModification, EMPTY_OPERATIONAL); diff --git a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/WriteTransaction.java b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/WriteTransaction.java index 2cbe1ec8b..93043ce07 100644 --- a/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/WriteTransaction.java +++ b/infra/data-impl/src/main/java/io/fd/honeycomb/data/impl/WriteTransaction.java @@ -142,7 +142,7 @@ final class WriteTransaction implements DOMDataWriteTransaction { status = COMMITED; } catch (DataValidationFailedException | TranslationException e) { status = FAILED; - LOG.error("Failed modify data tree", e); + LOG.debug("Submit failed", e); return Futures.immediateFailedCheckedFuture( new TransactionCommitFailedException("Failed to validate DataTreeModification", e)); } diff --git a/infra/data-impl/src/test/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegatorTest.java b/infra/data-impl/src/test/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegatorTest.java index 0a7c85b3b..322328508 100644 --- a/infra/data-impl/src/test/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegatorTest.java +++ b/infra/data-impl/src/test/java/io/fd/honeycomb/data/impl/ModifiableDataTreeDelegatorTest.java @@ -49,6 +49,8 @@ import java.util.HashMap; import java.util.Map; import org.junit.Before; import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; import org.mockito.Mock; import org.opendaylight.controller.md.sal.binding.api.DataBroker; import org.opendaylight.controller.md.sal.common.api.data.ReadFailedException; @@ -76,6 +78,9 @@ public class ModifiableDataTreeDelegatorTest { @Mock private org.opendaylight.controller.md.sal.binding.api.ReadWriteTransaction tx; + @Captor + private ArgumentCaptor writeContextCaptor; + private ModifiableDataTreeManager configDataTree; static final InstanceIdentifier DEFAULT_ID = InstanceIdentifier.create(DataObject.class); @@ -149,12 +154,11 @@ public class ModifiableDataTreeDelegatorTest { dataModification.write(ModificationDiffTest.NESTED_LIST_ID, nestedList); dataModification.validate(); dataModification.commit(); - fail("WriterRegistry.BulkUpdateException was expected"); - } catch (WriterRegistry.BulkUpdateException e) { + fail("WriterRegistry.RevertSuccessException was expected"); + } catch (WriterRegistry.Reverter.RevertSuccessException e) { verify(writer).update(any(WriterRegistry.DataObjectUpdates.class), any(WriteContext.class)); assertThat(e.getFailedIds(), hasItem(DEFAULT_ID)); - verify(reverter).revert(); - assertEquals(failedOnUpdateException, e.getCause()); + verify(reverter).revert(any(WriteContext.class)); } } @@ -171,7 +175,7 @@ public class ModifiableDataTreeDelegatorTest { // Fail on revert: final TranslationException failedOnRevertException = new TranslationException("revert failed"); doThrow(new WriterRegistry.Reverter.RevertFailedException(Collections.emptySet(), failedOnRevertException)) - .when(reverter).revert(); + .when(reverter).revert(any(WriteContext.class)); try { // Run the test @@ -182,7 +186,7 @@ public class ModifiableDataTreeDelegatorTest { fail("WriterRegistry.Reverter.RevertFailedException was expected"); } catch (WriterRegistry.Reverter.RevertFailedException e) { verify(writer).update(any(WriterRegistry.DataObjectUpdates.class), any(WriteContext.class)); - verify(reverter).revert(); + verify(reverter).revert(any(WriteContext.class)); assertEquals(failedOnRevertException, e.getCause()); } } -- cgit 1.2.3-korg