From 0afde20cdabcedb2bd8ddd1d59d12d3a5669600b Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Mon, 14 Apr 2014 17:46:07 +0200 Subject: [PATCH] Bug 509: Fixed small discrepancies in Binding Data Change Events. DataNormalizer which was responsible to render representation of Instance Identifier in original format was introducing additional node, when translation happened from normalized to legacy, which is only triggered by Data Change Events. Implementation of responsible algorithm was changed to use schema in order to derive legacy InstanceIdentifier. Data Change Events - Translated Data Change events were deserialized from DOM format on each access to fields, which may introduced CPU time overhead. Immutability of backing Data Change Events allows to add lazy caching of translated TOs, since event and data contained in Event Object are immutable. - getUpdated*Data in original Binding APIs returned both created and modified, so backwards-compatible fix was introduced. Change-Id: Idab3dffb8651d50cca79a22ddcd43af29561b80e Signed-off-by: Tony Tkacik Signed-off-by: Robert Varga --- .../impl/AbstractForwardedDataBroker.java | 116 +++++++++++------- .../impl/BindingToNormalizedNodeCodec.java | 19 ++- .../binding/impl/LegacyDataChangeEvent.java | 23 +++- .../impl/util/compat/DataNormalizer.java | 22 +--- 4 files changed, 115 insertions(+), 65 deletions(-) diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/AbstractForwardedDataBroker.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/AbstractForwardedDataBroker.java index 8a32b0b302..685a91979c 100644 --- a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/AbstractForwardedDataBroker.java +++ b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/AbstractForwardedDataBroker.java @@ -7,13 +7,13 @@ */ package org.opendaylight.controller.md.sal.binding.impl; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Map.Entry; import java.util.Set; -import org.eclipse.xtext.xbase.lib.Exceptions; import org.opendaylight.controller.md.sal.binding.api.BindingDataChangeListener; import org.opendaylight.controller.md.sal.common.api.data.AsyncDataBroker.DataChangeScope; import org.opendaylight.controller.md.sal.common.api.data.AsyncDataChangeEvent; @@ -36,7 +36,11 @@ import org.opendaylight.yangtools.yang.model.api.SchemaContextListener; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public abstract class AbstractForwardedDataBroker implements Delegator, DomForwardedBroker, SchemaContextListener { +import com.google.common.base.Objects; +import com.google.common.base.Optional; + +public abstract class AbstractForwardedDataBroker implements Delegator, DomForwardedBroker, + SchemaContextListener { private static final Logger LOG = LoggerFactory.getLogger(AbstractForwardedDataBroker.class); // The Broker to whom we do all forwarding @@ -81,11 +85,12 @@ public abstract class AbstractForwardedDataBroker implements Delegator domRegistration = domDataBroker.registerDataChangeListener(store, domPath, domDataChangeListener, triggeringScope); + ListenerRegistration domRegistration = domDataBroker.registerDataChangeListener(store, + domPath, domDataChangeListener, triggeringScope); return new ListenerRegistrationImpl(listener, domRegistration); } - protected Map, DataObject> fromDOMToData( + protected Map, DataObject> toBinding( final Map> normalized) { Map, DataObject> newMap = new HashMap<>(); for (Map.Entry> entry : normalized @@ -94,12 +99,38 @@ public abstract class AbstractForwardedDataBroker implements Delegator, DataObject> binding = getCodec().toBinding(entry); newMap.put(binding.getKey(), binding.getValue()); } catch (DeserializationException e) { - LOG.debug("Ommiting {}",entry,e); + LOG.debug("Omitting {}", entry, e); } } return newMap; } + protected Set> toBinding( + final Set normalized) { + Set> hashSet = new HashSet<>(); + for (org.opendaylight.yangtools.yang.data.api.InstanceIdentifier normalizedPath : normalized) { + try { + InstanceIdentifier binding = getCodec().toBinding(normalizedPath); + hashSet.add(binding); + } catch (DeserializationException e) { + LOG.debug("Omitting {}", normalizedPath, e); + } + } + return hashSet; + } + + protected Optional toBindingData(final InstanceIdentifier path, final NormalizedNode data) { + if(path.isWildcarded()) { + return Optional.absent(); + } + + try { + return Optional.fromNullable(getCodec().toBinding(path, data)); + } catch (DeserializationException e) { + return Optional.absent(); + } + } + private class TranslatingDataChangeInvoker implements DOMDataChangeListener { private final BindingDataChangeListener bindingDataChangeListener; private final LogicalDatastoreType store; @@ -117,18 +148,20 @@ public abstract class AbstractForwardedDataBroker implements Delegator> change) { - bindingDataChangeListener.onDataChanged(new TranslatedDataChangeEvent(change,path)); + bindingDataChangeListener.onDataChanged(new TranslatedDataChangeEvent(change, path)); } } private class TranslatedDataChangeEvent implements AsyncDataChangeEvent, DataObject> { private final AsyncDataChangeEvent> domEvent; - private InstanceIdentifier path; + private final InstanceIdentifier path; - public TranslatedDataChangeEvent( - final AsyncDataChangeEvent> change) { - this.domEvent = change; - } + private Map, DataObject> createdCache; + private Map, DataObject> updatedCache; + private Map, ? extends DataObject> originalCache; + private Set> removedCache; + private Optional originalDataCache; + private Optional updatedDataCache; public TranslatedDataChangeEvent( final AsyncDataChangeEvent> change, @@ -139,52 +172,63 @@ public abstract class AbstractForwardedDataBroker implements Delegator, DataObject> getCreatedData() { - return fromDOMToData(domEvent.getCreatedData()); + if (createdCache == null) { + createdCache = Collections.unmodifiableMap(toBinding(domEvent.getCreatedData())); + } + return createdCache; } @Override public Map, DataObject> getUpdatedData() { - return fromDOMToData(domEvent.getUpdatedData()); + if (updatedCache == null) { + updatedCache = Collections.unmodifiableMap(toBinding(domEvent.getUpdatedData())); + } + return updatedCache; } @Override public Set> getRemovedPaths() { - final Set removedPaths = domEvent - .getRemovedPaths(); - final Set> output = new HashSet<>(); - for (org.opendaylight.yangtools.yang.data.api.InstanceIdentifier instanceIdentifier : removedPaths) { - try { - output.add(mappingService.fromDataDom(instanceIdentifier)); - } catch (DeserializationException e) { - Exceptions.sneakyThrow(e); - } + if (removedCache == null) { + removedCache = Collections.unmodifiableSet(toBinding(domEvent.getRemovedPaths())); } - - return output; + return removedCache; } @Override public Map, ? extends DataObject> getOriginalData() { - return fromDOMToData(domEvent.getOriginalData()); + if (originalCache == null) { + originalCache = Collections.unmodifiableMap(toBinding(domEvent.getOriginalData())); + } + return originalCache; } @Override public DataObject getOriginalSubtree() { - - return toBindingData(path,domEvent.getOriginalSubtree()); + if (originalDataCache == null) { + originalDataCache = toBindingData(path, domEvent.getOriginalSubtree()); + } + return originalDataCache.orNull(); } @Override public DataObject getUpdatedSubtree() { + if (updatedDataCache == null) { + updatedDataCache = toBindingData(path, domEvent.getUpdatedSubtree()); + } - return toBindingData(path,domEvent.getUpdatedSubtree()); + return updatedDataCache.orNull(); } @Override public String toString() { - return "TranslatedDataChangeEvent [domEvent=" + domEvent + "]"; + return Objects.toStringHelper(TranslatedDataChangeEvent.class) // + .add("created", getCreatedData()) // + .add("updated", getUpdatedData()) // + .add("removed", getRemovedPaths()) // + .add("dom", domEvent) // + .toString(); } } @@ -203,15 +247,6 @@ public abstract class AbstractForwardedDataBroker implements Delegator path, final NormalizedNode data) { - try { - return getCodec().toBinding(path, data); - } catch (DeserializationException e) { - return null; - } - } - - @Override public BindingIndependentConnector getConnector() { return this.connector; @@ -229,7 +264,7 @@ public abstract class AbstractForwardedDataBroker implements Delegator binding) { - return legacyToNormalized.toNormalized(bindingToLegacy.toDataDom(binding)); + final org.opendaylight.yangtools.yang.data.api.InstanceIdentifier legacyPath = bindingToLegacy.toDataDom(binding); + final org.opendaylight.yangtools.yang.data.api.InstanceIdentifier normalized = legacyToNormalized.toNormalized(legacyPath); + LOG.trace("InstanceIdentifier Path {} Serialization: Legacy representation {}, Normalized representation: {}",binding,legacyPath,normalized); + return normalized; } public Entry> toNormalizedNode( @@ -54,7 +58,9 @@ public class BindingToNormalizedNodeCodec implements SchemaContextListener { public Entry> toNormalizedNode( final Entry, DataObject> binding) { - Entry> normalizedEntry = legacyToNormalized.toNormalized(bindingToLegacy.toDataDom(binding)); + Entry legacyEntry = bindingToLegacy.toDataDom(binding); + Entry> normalizedEntry = legacyToNormalized.toNormalized(legacyEntry); + LOG.trace("Serialization of {}, Legacy Representation: {}, Normalized Representation: {}",binding,legacyEntry,normalizedEntry); if(Augmentation.class.isAssignableFrom(binding.getKey().getTargetType())) { for(DataContainerChild child : ((DataContainerNode) normalizedEntry.getValue()).getValue()) { @@ -78,8 +84,13 @@ public class BindingToNormalizedNodeCodec implements SchemaContextListener { final org.opendaylight.yangtools.yang.data.api.InstanceIdentifier normalized) throws DeserializationException { - org.opendaylight.yangtools.yang.data.api.InstanceIdentifier legacyPath = legacyToNormalized - .toLegacy(normalized); + org.opendaylight.yangtools.yang.data.api.InstanceIdentifier legacyPath; + try { + legacyPath = legacyToNormalized.toLegacy(normalized); + } catch (DataNormalizationException e) { + throw new IllegalStateException("Could not denormalize path.",e); + } + LOG.trace("InstanceIdentifier Path Deserialization: Legacy representation {}, Normalized representation: {}",legacyPath,normalized); return bindingToLegacy.fromDataDom(legacyPath); } diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/LegacyDataChangeEvent.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/LegacyDataChangeEvent.java index 8cb4a70f9c..ce1ff450c9 100644 --- a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/LegacyDataChangeEvent.java +++ b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/LegacyDataChangeEvent.java @@ -8,6 +8,7 @@ package org.opendaylight.controller.md.sal.binding.impl; import java.util.Collections; +import java.util.HashMap; import java.util.Map; import java.util.Set; @@ -96,6 +97,7 @@ public abstract class LegacyDataChangeEvent implements private final static class OperationalChangeEvent extends LegacyDataChangeEvent { private final AsyncDataChangeEvent, DataObject> delegate; + private Map, DataObject> updatedCache; public OperationalChangeEvent(final AsyncDataChangeEvent, DataObject> change) { this.delegate = change; @@ -128,7 +130,15 @@ public abstract class LegacyDataChangeEvent implements @Override public Map, DataObject> getUpdatedOperationalData() { - return delegate.getUpdatedData(); + if(updatedCache == null) { + Map, DataObject> created = delegate.getCreatedData(); + Map, DataObject> updated = delegate.getUpdatedData(); + HashMap, DataObject> updatedComposite = new HashMap<>(created.size() + updated.size()); + updatedComposite.putAll(created); + updatedComposite.putAll(updated); + updatedCache = Collections.unmodifiableMap(updatedComposite); + } + return updatedCache; } @Override @@ -142,6 +152,7 @@ public abstract class LegacyDataChangeEvent implements private final static class ConfigurationChangeEvent extends LegacyDataChangeEvent { private final AsyncDataChangeEvent, DataObject> delegate; + private Map, DataObject> updatedCache; public ConfigurationChangeEvent(final AsyncDataChangeEvent, DataObject> change) { this.delegate = change; @@ -174,7 +185,15 @@ public abstract class LegacyDataChangeEvent implements @Override public Map, DataObject> getUpdatedConfigurationData() { - return delegate.getUpdatedData(); + if(updatedCache == null) { + Map, DataObject> created = delegate.getCreatedData(); + Map, DataObject> updated = delegate.getUpdatedData(); + HashMap, DataObject> updatedComposite = new HashMap<>(created.size() + updated.size()); + updatedComposite.putAll(created); + updatedComposite.putAll(updated); + updatedCache = Collections.unmodifiableMap(updatedComposite); + } + return updatedCache; } @Override diff --git a/opendaylight/md-sal/sal-common-impl/src/main/java/org/opendaylight/controller/md/sal/common/impl/util/compat/DataNormalizer.java b/opendaylight/md-sal/sal-common-impl/src/main/java/org/opendaylight/controller/md/sal/common/impl/util/compat/DataNormalizer.java index e52e196326..8fb6ff38a2 100644 --- a/opendaylight/md-sal/sal-common-impl/src/main/java/org/opendaylight/controller/md/sal/common/impl/util/compat/DataNormalizer.java +++ b/opendaylight/md-sal/sal-common-impl/src/main/java/org/opendaylight/controller/md/sal/common/impl/util/compat/DataNormalizer.java @@ -18,8 +18,6 @@ import org.opendaylight.yangtools.yang.common.QName; import org.opendaylight.yangtools.yang.data.api.CompositeNode; import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier; import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.AugmentationIdentifier; -import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.NodeIdentifier; -import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.NodeIdentifierWithPredicates; import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.PathArgument; import org.opendaylight.yangtools.yang.data.api.Node; import org.opendaylight.yangtools.yang.data.api.SimpleNode; @@ -117,25 +115,15 @@ public class DataNormalizer { currentOp.normalize(legacyData)); } - public InstanceIdentifier toLegacy(final InstanceIdentifier normalized) { + public InstanceIdentifier toLegacy(final InstanceIdentifier normalized) throws DataNormalizationException { ImmutableList.Builder legacyArgs = ImmutableList.builder(); PathArgument previous = null; + DataNormalizationOperation currentOp = operation; for (PathArgument normalizedArg : normalized.getPath()) { - if (normalizedArg instanceof NodeIdentifier) { - if (previous != null) { - legacyArgs.add(previous); - } - previous = normalizedArg; - } else if (normalizedArg instanceof NodeIdentifierWithPredicates) { - // We skip previous node, which was mixin. - previous = normalizedArg; - } else if (normalizedArg instanceof AugmentationIdentifier) { - // We ignore argument + currentOp = currentOp.getChild(normalizedArg); + if(!currentOp.isMixin()) { + legacyArgs.add(normalizedArg); } - // FIXME : Add option for reading choice - } - if (previous != null) { - legacyArgs.add(previous); } return new InstanceIdentifier(legacyArgs.build()); } -- 2.36.6