Fixed order of applying changes when forwarding transactions

Commit 5659f588f80522a5be5c80d0e5ff34068b5f4786 changed the semantics of
remove/update operations such that the order of application became
significant: remove, then put.

Unfortunately we failed to notice that there are two places which apply
the operations in wrong order, thus breaking clients who perform a
remove/put in a single transaction -- e.g. replacing data without
merging it.

Fix DataStore and Binding->DOM connector.

Change-Id: I4fb12730e8256d3614846adb1fa5a1096284e0eb
Signed-off-by: Robert Varga <rovarga@cisco.com>
Signed-off-by: Tony Tkacik <ttkacik@cisco.com>

diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/impl/connect/dom/BindingIndependentConnector.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/impl/connect/dom/BindingIndependentConnector.java
index e48ebbc0577f1b6101a772284ba9e07b3e6580cf..e6e935c920b613c577d4b9f890092a22c13cb49d 100644 (file)
--- a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/impl/connect/dom/BindingIndependentConnector.java
+++ b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/sal/binding/impl/connect/dom/BindingIndependentConnector.java
@@ -202,6 +202,16 @@ public class BindingIndependentConnector implements //
             DataModification<InstanceIdentifier<? extends DataObject>, DataObject> source) {
         DataModificationTransaction target = biDataService.beginTransaction();
         LOG.debug("Created DOM Transaction {} for {},", target.getIdentifier(),source.getIdentifier());
+        for (InstanceIdentifier<? extends DataObject> entry : source.getRemovedConfigurationData()) {
+            org.opendaylight.yangtools.yang.data.api.InstanceIdentifier biEntry = mappingService.toDataDom(entry);
+            target.removeConfigurationData(biEntry);
+            LOG.debug("Delete of Binding Configuration Data {} is translated to {}",entry,biEntry);
+        }
+        for (InstanceIdentifier<? extends DataObject> entry : source.getRemovedOperationalData()) {
+            org.opendaylight.yangtools.yang.data.api.InstanceIdentifier biEntry = mappingService.toDataDom(entry);
+            target.removeOperationalData(biEntry);
+            LOG.debug("Delete of Binding Operational Data {} is translated to {}",entry,biEntry);
+        }
         for (Entry<InstanceIdentifier<? extends DataObject>, DataObject> entry : source.getUpdatedConfigurationData()
                 .entrySet()) {
             Entry<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, CompositeNode> biEntry = mappingService
@@ -216,16 +226,7 @@ public class BindingIndependentConnector implements //
             target.putOperationalData(biEntry.getKey(), biEntry.getValue());
             LOG.debug("Update of Binding Operational Data {} is translated to {}",entry,biEntry);
         }
-        for (InstanceIdentifier<? extends DataObject> entry : source.getRemovedConfigurationData()) {
-            org.opendaylight.yangtools.yang.data.api.InstanceIdentifier biEntry = mappingService.toDataDom(entry);
-            target.removeConfigurationData(biEntry);
-            LOG.debug("Delete of Binding Configuration Data {} is translated to {}",entry,biEntry);
-        }
-        for (InstanceIdentifier<? extends DataObject> entry : source.getRemovedOperationalData()) {
-            org.opendaylight.yangtools.yang.data.api.InstanceIdentifier biEntry = mappingService.toDataDom(entry);
-            target.removeOperationalData(biEntry);
-            LOG.debug("Delete of Binding Operational Data {} is translated to {}",entry,biEntry);
-        }
+
         return target;
     }
 
@@ -233,6 +234,24 @@ public class BindingIndependentConnector implements //
             DataModification<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, CompositeNode> source) {
         org.opendaylight.controller.sal.binding.api.data.DataModificationTransaction target = baDataService
                 .beginTransaction();
+        for (org.opendaylight.yangtools.yang.data.api.InstanceIdentifier entry : source.getRemovedConfigurationData()) {
+            try {
+
+                InstanceIdentifier<?> baEntry = mappingService.fromDataDom(entry);
+                target.removeConfigurationData(baEntry);
+            } catch (DeserializationException e) {
+                LOG.error("Ommiting from BA transaction: {}.", entry, e);
+            }
+        }
+        for (org.opendaylight.yangtools.yang.data.api.InstanceIdentifier entry : source.getRemovedOperationalData()) {
+            try {
+
+                InstanceIdentifier<?> baEntry = mappingService.fromDataDom(entry);
+                target.removeOperationalData(baEntry);
+            } catch (DeserializationException e) {
+                LOG.error("Ommiting from BA transaction: {}.", entry, e);
+            }
+        }
         for (Entry<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, CompositeNode> entry : source
                 .getUpdatedConfigurationData().entrySet()) {
             try {
@@ -254,24 +273,6 @@ public class BindingIndependentConnector implements //
                 LOG.error("Ommiting from BA transaction: {}.", entry.getKey(), e);
             }
         }
-        for (org.opendaylight.yangtools.yang.data.api.InstanceIdentifier entry : source.getRemovedConfigurationData()) {
-            try {
-
-                InstanceIdentifier<?> baEntry = mappingService.fromDataDom(entry);
-                target.removeConfigurationData(baEntry);
-            } catch (DeserializationException e) {
-                LOG.error("Ommiting from BA transaction: {}.", entry, e);
-            }
-        }
-        for (org.opendaylight.yangtools.yang.data.api.InstanceIdentifier entry : source.getRemovedOperationalData()) {
-            try {
-
-                InstanceIdentifier<?> baEntry = mappingService.fromDataDom(entry);
-                target.removeOperationalData(baEntry);
-            } catch (DeserializationException e) {
-                LOG.error("Ommiting from BA transaction: {}.", entry, e);
-            }
-        }
         return target;
     }
 

diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/sal/dom/broker/BrokerConfigActivator.xtend b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/sal/dom/broker/BrokerConfigActivator.xtend
index 1159d5650e6599a72db0af912909244d17cf612b..6b5f5acb1945a872d48b925da58283e314014af4 100644 (file)
--- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/sal/dom/broker/BrokerConfigActivator.xtend
+++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/sal/dom/broker/BrokerConfigActivator.xtend
@@ -50,7 +50,7 @@ class BrokerConfigActivator implements AutoCloseable {
         broker.setRouter(new SchemaAwareRpcBroker("/", SchemaContextProviders.fromSchemaService(schemaService)));
 
         dataService = new DataBrokerImpl();
-        dataService.setExecutor(broker.getExecutor());
+        //dataService.setExecutor(broker.getExecutor());
 
         dataReg = context.registerService(DataBrokerService, dataService, emptyProperties);
         dataProviderReg = context.registerService(DataProviderService, dataService, emptyProperties);

diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/sal/dom/broker/impl/SchemaAwareDataStoreAdapter.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/sal/dom/broker/impl/SchemaAwareDataStoreAdapter.java
index f380c273732c5fb174c51a07cc2f554b21f394c0..d8315568bee86ea07ee00b86feddcb0801faaa8e 100644 (file)
--- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/sal/dom/broker/impl/SchemaAwareDataStoreAdapter.java
+++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/sal/dom/broker/impl/SchemaAwareDataStoreAdapter.java
@@ -9,19 +9,14 @@ package org.opendaylight.controller.sal.dom.broker.impl;
 
 import static com.google.common.base.Preconditions.checkState;
 
-import java.io.Console;
 import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.HashMap;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
-import java.util.Set;
 import java.util.concurrent.Future;
 
-import javax.activation.UnsupportedDataTypeException;
-
 import org.opendaylight.controller.md.sal.common.api.TransactionStatus;
 import org.opendaylight.controller.md.sal.common.api.data.DataModification;
 import org.opendaylight.controller.md.sal.common.api.data.DataReader;
@@ -34,11 +29,9 @@ import org.opendaylight.yangtools.yang.common.QName;
 import org.opendaylight.yangtools.yang.common.RpcResult;
 import org.opendaylight.yangtools.yang.data.api.CompositeNode;
 import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier;
-import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.InstanceIdentifierBuilder;
 import org.opendaylight.yangtools.yang.data.api.Node;
 import org.opendaylight.yangtools.yang.data.api.SimpleNode;
 import org.opendaylight.yangtools.yang.data.impl.CompositeNodeTOImpl;
-import org.opendaylight.yangtools.yang.model.api.ConstraintDefinition;
 import org.opendaylight.yangtools.yang.model.api.ContainerSchemaNode;
 import org.opendaylight.yangtools.yang.model.api.DataSchemaNode;
 import org.opendaylight.yangtools.yang.model.api.ListSchemaNode;
@@ -47,7 +40,6 @@ import org.opendaylight.yangtools.yang.model.api.SchemaContextListener;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.google.common.base.Optional;
 import com.google.common.base.Predicate;
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableSet;
@@ -180,18 +172,25 @@ public class SchemaAwareDataStoreAdapter extends AbstractLockableDelegator<DataS
     private NormalizedDataModification prepareMergedTransaction(
             DataModification<InstanceIdentifier, CompositeNode> original) {
         NormalizedDataModification normalized = new NormalizedDataModification(original);
-        for (Entry<InstanceIdentifier, CompositeNode> entry : original.getUpdatedConfigurationData().entrySet()) {
-            normalized.putDeepConfigurationData(entry.getKey(), entry.getValue());
-        }
-        for (Entry<InstanceIdentifier, CompositeNode> entry : original.getUpdatedOperationalData().entrySet()) {
-            normalized.putDeepOperationalData(entry.getKey(), entry.getValue());
-        }
+        LOG.trace("Transaction: {} Removed Configuration {}, Removed Operational {}", original.getIdentifier(),
+                original.getRemovedConfigurationData(), original.getRemovedConfigurationData());
+        LOG.trace("Transaction: {} Created Configuration {}, Created Operational {}", original.getIdentifier(),
+                original.getCreatedConfigurationData().entrySet(), original.getCreatedOperationalData().entrySet());
+        LOG.trace("Transaction: {} Updated Configuration {}, Updated Operational {}", original.getIdentifier(),
+                original.getUpdatedConfigurationData().entrySet(), original.getUpdatedOperationalData().entrySet());
+
         for (InstanceIdentifier entry : original.getRemovedConfigurationData()) {
             normalized.deepRemoveConfigurationData(entry);
         }
         for (InstanceIdentifier entry : original.getRemovedOperationalData()) {
             normalized.deepRemoveOperationalData(entry);
         }
+        for (Entry<InstanceIdentifier, CompositeNode> entry : original.getUpdatedConfigurationData().entrySet()) {
+            normalized.putDeepConfigurationData(entry.getKey(), entry.getValue());
+        }
+        for (Entry<InstanceIdentifier, CompositeNode> entry : original.getUpdatedOperationalData().entrySet()) {
+            normalized.putDeepOperationalData(entry.getKey(), entry.getValue());
+        }
         return normalized;
     }
 
@@ -359,7 +358,7 @@ public class SchemaAwareDataStoreAdapter extends AbstractLockableDelegator<DataS
         public void putDeepConfigurationData(InstanceIdentifier entryKey, CompositeNode entryData) {
             this.putCompositeNodeData(entryKey, entryData, CONFIGURATIONAL_DATA_STORE_MARKER);
         }
-        
+
         public void putDeepOperationalData(InstanceIdentifier entryKey, CompositeNode entryData) {
             this.putCompositeNodeData(entryKey, entryData, OPERATIONAL_DATA_STORE_MARKER);
         }
@@ -401,18 +400,20 @@ public class SchemaAwareDataStoreAdapter extends AbstractLockableDelegator<DataS
                     this.putOperationalData(entryKey, entryData);
                     break;
 
-                default :
+                default:
                     LOG.error(dataStoreIdentifier + " is NOT valid DataStore switch marker");
                     throw new RuntimeException(dataStoreIdentifier + " is NOT valid DataStore switch marker");
                 }
             }
         }
 
-        private void putCompositeNodeData(InstanceIdentifier entryKey, CompositeNode entryData, String dataStoreIdentifier) {
+        private void putCompositeNodeData(InstanceIdentifier entryKey, CompositeNode entryData,
+                String dataStoreIdentifier) {
             this.putData(entryKey, entryData, dataStoreIdentifier);
-            
+
             for (Node<?> child : entryData.getChildren()) {
-                InstanceIdentifier subEntryId = InstanceIdentifier.builder(entryKey).node(child.getNodeType()).toInstance();
+                InstanceIdentifier subEntryId = InstanceIdentifier.builder(entryKey).node(child.getNodeType())
+                        .toInstance();
                 if (child instanceof CompositeNode) {
                     DataSchemaNode subSchema = schemaNodeFor(subEntryId);
                     CompositeNode compNode = (CompositeNode) child;
@@ -420,12 +421,13 @@ public class SchemaAwareDataStoreAdapter extends AbstractLockableDelegator<DataS
 
                     if (subSchema instanceof ListSchemaNode) {
                         ListSchemaNode listSubSchema = (ListSchemaNode) subSchema;
-                        Map<QName, Object> mapOfSubValues = this.getValuesFromListSchema(listSubSchema, (CompositeNode) child);
+                        Map<QName, Object> mapOfSubValues = this.getValuesFromListSchema(listSubSchema,
+                                (CompositeNode) child);
                         if (mapOfSubValues != null) {
-                            instanceId = InstanceIdentifier.builder(entryKey).nodeWithKey(listSubSchema.getQName(), mapOfSubValues).toInstance();
+                            instanceId = InstanceIdentifier.builder(entryKey)
+                                    .nodeWithKey(listSubSchema.getQName(), mapOfSubValues).toInstance();
                         }
-                    } 
-                    else if (subSchema instanceof ContainerSchemaNode) {
+                    } else if (subSchema instanceof ContainerSchemaNode) {
                         ContainerSchemaNode containerSchema = (ContainerSchemaNode) subSchema;
                         instanceId = InstanceIdentifier.builder(entryKey).node(subSchema.getQName()).toInstance();
                     }
@@ -436,13 +438,13 @@ public class SchemaAwareDataStoreAdapter extends AbstractLockableDelegator<DataS
             }
         }
 
-        private Map<QName, Object> getValuesFromListSchema (ListSchemaNode listSchema, CompositeNode entryData) {
+        private Map<QName, Object> getValuesFromListSchema(ListSchemaNode listSchema, CompositeNode entryData) {
             List<QName> keyDef = listSchema.getKeyDefinition();
-            if (keyDef != null && ! keyDef.isEmpty()) {
+            if (keyDef != null && !keyDef.isEmpty()) {
                 Map<QName, Object> map = new HashMap<QName, Object>();
                 for (QName key : keyDef) {
                     List<Node<?>> data = entryData.get(key);
-                    if (data != null && ! data.isEmpty()) {
+                    if (data != null && !data.isEmpty()) {
                         for (Node<?> nodeData : data) {
                             if (nodeData instanceof SimpleNode<?>) {
                                 map.put(key, data.get(0).getValue());