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 <ttkacik@cisco.com>

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 ee7607306a8aa9f242a50c64b81237fe61132756..ab0dc6e24fb91c96ebf6e40aaa2b6251489a3c2b 100644 (file)
--- 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<InstanceIdentifier<? extends DataObject>, DataObject> original = new HashMap<>();
         private TransactionStatus status = TransactionStatus.NEW;
 
+        private final Set<InstanceIdentifier<? extends DataObject>> posponedRemovedOperational = new HashSet<>();
+        private final Set<InstanceIdentifier<? extends DataObject>> 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<? extends DataObject> path, final DataObject data) {
-
+            posponedRemovedOperational.remove(path);
             doPutWithEnsureParents(getDelegate(), LogicalDatastoreType.OPERATIONAL, path, data);
         }
 
         @Override
         public void putConfigurationData(final InstanceIdentifier<? extends DataObject> 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<? extends DataObject> path) {
-            doDelete(getDelegate(), LogicalDatastoreType.OPERATIONAL, path);
-
+            posponedRemovedOperational.add(path);
         }
 
         @Override
         public void removeConfigurationData(final InstanceIdentifier<? extends DataObject> 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<RpcResult<TransactionStatus>> commit() {
+
+            for(InstanceIdentifier<? extends DataObject> path : posponedRemovedConfiguration) {
+                doDelete(getDelegate(), LogicalDatastoreType.CONFIGURATION, path);
+            }
+
+            for(InstanceIdentifier<? extends DataObject> path : posponedRemovedOperational) {
+                doDelete(getDelegate(), LogicalDatastoreType.OPERATIONAL, path);
+            }
+
             final ListenableFuture<RpcResult<TransactionStatus>> f = ForwardedBackwardsCompatibleDataBroker.this.commit(this);
 
             changeStatus(TransactionStatus.SUBMITED);
 
             Futures.addCallback(f, new FutureCallback<RpcResult<TransactionStatus>>() {
                 @Override
-                public void onSuccess(RpcResult<TransactionStatus> result) {
+                public void onSuccess(final RpcResult<TransactionStatus> 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 86f08de61566d6da785fe49166cdde694e8ed92a..3dfca40b4045d435cb79e909a7be0c9b77cec248 100644 (file)
--- 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<InstanceIdentifier, NormalizedNode<?, ?>> {
@@ -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<InstanceIdentifier, NormalizedNode<?, ?>> original = ImmutableMap.builder();
-        private final ImmutableMap.Builder<InstanceIdentifier, NormalizedNode<?, ?>> created = ImmutableMap.builder();
-        private final ImmutableMap.Builder<InstanceIdentifier, NormalizedNode<?, ?>> updated = ImmutableMap.builder();
-        private final ImmutableSet.Builder<InstanceIdentifier> removed = ImmutableSet.builder();
+        private final Map<InstanceIdentifier, NormalizedNode<?, ?>> original = new HashMap<>();
+        private final Map<InstanceIdentifier, NormalizedNode<?, ?>> created = new HashMap<>();
+        private final Map<InstanceIdentifier, NormalizedNode<?, ?>> updated = new HashMap<>();
+        private final Set<InstanceIdentifier> removed = new HashSet<>();
 
         private Builder(final DataChangeScope scope) {
             Preconditions.checkNotNull(scope, "Data change scope should not be null.");