From b98d04e31ad212b8549a3cf849ce864bbba90717 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Thu, 27 Feb 2014 12:44:18 +0100 Subject: [PATCH] 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 Signed-off-by: Tony Tkacik --- .../dom/BindingIndependentConnector.java | 57 ++++++++++--------- .../dom/broker/BrokerConfigActivator.xtend | 2 +- .../impl/SchemaAwareDataStoreAdapter.java | 54 +++++++++--------- 3 files changed, 58 insertions(+), 55 deletions(-) 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 e48ebbc057..e6e935c920 100644 --- 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, DataObject> source) { DataModificationTransaction target = biDataService.beginTransaction(); LOG.debug("Created DOM Transaction {} for {},", target.getIdentifier(),source.getIdentifier()); + for (InstanceIdentifier 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 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, DataObject> entry : source.getUpdatedConfigurationData() .entrySet()) { Entry 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 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 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 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 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 1159d5650e..6b5f5acb19 100644 --- 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 f380c27373..d8315568be 100644 --- 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 original) { NormalizedDataModification normalized = new NormalizedDataModification(original); - for (Entry entry : original.getUpdatedConfigurationData().entrySet()) { - normalized.putDeepConfigurationData(entry.getKey(), entry.getValue()); - } - for (Entry 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 entry : original.getUpdatedConfigurationData().entrySet()) { + normalized.putDeepConfigurationData(entry.getKey(), entry.getValue()); + } + for (Entry entry : original.getUpdatedOperationalData().entrySet()) { + normalized.putDeepOperationalData(entry.getKey(), entry.getValue()); + } return normalized; } @@ -359,7 +358,7 @@ public class SchemaAwareDataStoreAdapter extends AbstractLockableDelegator 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 mapOfSubValues = this.getValuesFromListSchema(listSubSchema, (CompositeNode) child); + Map 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 getValuesFromListSchema (ListSchemaNode listSchema, CompositeNode entryData) { + private Map getValuesFromListSchema(ListSchemaNode listSchema, CompositeNode entryData) { List keyDef = listSchema.getKeyDefinition(); - if (keyDef != null && ! keyDef.isEmpty()) { + if (keyDef != null && !keyDef.isEmpty()) { Map map = new HashMap(); for (QName key : keyDef) { List> 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()); -- 2.36.6