From: Tony Tkacik Date: Tue, 15 Apr 2014 15:48:56 +0000 (+0200) Subject: Bug 509: Fixed incorrect merging of Data Store Writes / Events X-Git-Tag: autorelease-tag-v20140601202136_82eb3f9~215^2 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=0c108e1b6b108b55b79c5b6dcdb0b4261ac510ba Bug 509: Fixed incorrect merging of Data Store Writes / Events ForwardedBackwardsCompatibleDataBroker: In old Data API only way to replace element was to use remove/put combination, which changed with new Async DOM Broker APIs where put is actually add/replace so previous delete is not neccessary. BackwardsCompatibleTransaction keeps track of such remove/put operations and propagates put-only to new Async Data Transaction. DOMImmutableDataChangeEvent: Builder for DOMImmutableDataChange event used ImmutableMap builder which throws IllegalArgumentException when same key/value combo is overriden, which made impossible to merge some events. Switched Builder implementation to use combination of HashMap and ImmutableMap.copyOf() in order to prevent this IllegalArgumentException to fail commit process. Change-Id: I935bd4f5499c26190c52adcaee1df6335640bfaf Signed-off-by: Tony Tkacik --- diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/ForwardedBackwardsCompatibleDataBroker.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/ForwardedBackwardsCompatibleDataBroker.java index ee7607306a..ab0dc6e24f 100644 --- a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/ForwardedBackwardsCompatibleDataBroker.java +++ b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/ForwardedBackwardsCompatibleDataBroker.java @@ -201,6 +201,10 @@ public class ForwardedBackwardsCompatibleDataBroker extends AbstractForwardedDat private final Map, DataObject> original = new HashMap<>(); private TransactionStatus status = TransactionStatus.NEW; + private final Set> posponedRemovedOperational = new HashSet<>(); + private final Set> posponedRemovedConfiguration = new HashSet<>(); + + @Override public final TransactionStatus getStatus() { return status; @@ -214,12 +218,13 @@ public class ForwardedBackwardsCompatibleDataBroker extends AbstractForwardedDat @Override public void putOperationalData(final InstanceIdentifier path, final DataObject data) { - + posponedRemovedOperational.remove(path); doPutWithEnsureParents(getDelegate(), LogicalDatastoreType.OPERATIONAL, path, data); } @Override public void putConfigurationData(final InstanceIdentifier path, final DataObject data) { + posponedRemovedConfiguration.remove(path); DataObject originalObj = readConfigurationData(path); if (originalObj != null) { original.put(path, originalObj); @@ -233,13 +238,12 @@ public class ForwardedBackwardsCompatibleDataBroker extends AbstractForwardedDat @Override public void removeOperationalData(final InstanceIdentifier path) { - doDelete(getDelegate(), LogicalDatastoreType.OPERATIONAL, path); - + posponedRemovedOperational.add(path); } @Override public void removeConfigurationData(final InstanceIdentifier path) { - doDelete(getDelegate(), LogicalDatastoreType.CONFIGURATION, path); + posponedRemovedConfiguration.add(path); } @Override @@ -307,25 +311,34 @@ public class ForwardedBackwardsCompatibleDataBroker extends AbstractForwardedDat return getDelegate().getIdentifier(); } - private void changeStatus(TransactionStatus status) { + private void changeStatus(final TransactionStatus status) { LOG.trace("Transaction {} changed status to {}", getIdentifier(), status); this.status = status; } @Override public ListenableFuture> commit() { + + for(InstanceIdentifier path : posponedRemovedConfiguration) { + doDelete(getDelegate(), LogicalDatastoreType.CONFIGURATION, path); + } + + for(InstanceIdentifier path : posponedRemovedOperational) { + doDelete(getDelegate(), LogicalDatastoreType.OPERATIONAL, path); + } + final ListenableFuture> f = ForwardedBackwardsCompatibleDataBroker.this.commit(this); changeStatus(TransactionStatus.SUBMITED); Futures.addCallback(f, new FutureCallback>() { @Override - public void onSuccess(RpcResult result) { + public void onSuccess(final RpcResult result) { changeStatus(result.getResult()); } @Override - public void onFailure(Throwable t) { + public void onFailure(final Throwable t) { LOG.error("Transaction {} failed to complete", getIdentifier(), t); changeStatus(TransactionStatus.FAILED); } diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/DOMImmutableDataChangeEvent.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/DOMImmutableDataChangeEvent.java index 86f08de615..3dfca40b40 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/DOMImmutableDataChangeEvent.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/DOMImmutableDataChangeEvent.java @@ -1,5 +1,8 @@ package org.opendaylight.controller.md.sal.dom.store.impl; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -10,8 +13,6 @@ import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.PathArgument; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; public final class DOMImmutableDataChangeEvent implements AsyncDataChangeEvent> { @@ -33,10 +34,10 @@ public final class DOMImmutableDataChangeEvent implements private DOMImmutableDataChangeEvent(final Builder change) { original = change.before; updated = change.after; - originalData = change.original.build(); - createdData = change.created.build(); - updatedData = change.updated.build(); - removedPaths = change.removed.build(); + originalData = Collections.unmodifiableMap(change.original); + createdData = Collections.unmodifiableMap(change.created); + updatedData = Collections.unmodifiableMap(change.updated); + removedPaths = Collections.unmodifiableSet(change.removed); scope = change.scope; } @@ -125,10 +126,10 @@ public final class DOMImmutableDataChangeEvent implements private NormalizedNode after; private NormalizedNode before; - private final ImmutableMap.Builder> original = ImmutableMap.builder(); - private final ImmutableMap.Builder> created = ImmutableMap.builder(); - private final ImmutableMap.Builder> updated = ImmutableMap.builder(); - private final ImmutableSet.Builder removed = ImmutableSet.builder(); + private final Map> original = new HashMap<>(); + private final Map> created = new HashMap<>(); + private final Map> updated = new HashMap<>(); + private final Set removed = new HashSet<>(); private Builder(final DataChangeScope scope) { Preconditions.checkNotNull(scope, "Data change scope should not be null.");