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 <ttkacik@cisco.com>
Signed-off-by: Robert Varga <rovarga@cisco.com>

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 8a32b0b3026f1d028dbab77a00e6c54be22d07ad..685a91979c7c9f8ea7c1493b047867622dfa8d81 100644 (file)
--- 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<DOMDataBroker>, DomForwardedBroker, SchemaContextListener {
+import com.google.common.base.Objects;
+import com.google.common.base.Optional;
+
+public abstract class AbstractForwardedDataBroker implements Delegator<DOMDataBroker>, 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<DOMDataBr
         DOMDataChangeListener domDataChangeListener = new TranslatingDataChangeInvoker(store, path, listener,
                 triggeringScope);
         org.opendaylight.yangtools.yang.data.api.InstanceIdentifier domPath = codec.toNormalized(path);
-        ListenerRegistration<DOMDataChangeListener> domRegistration = domDataBroker.registerDataChangeListener(store, domPath, domDataChangeListener, triggeringScope);
+        ListenerRegistration<DOMDataChangeListener> domRegistration = domDataBroker.registerDataChangeListener(store,
+                domPath, domDataChangeListener, triggeringScope);
         return new ListenerRegistrationImpl(listener, domRegistration);
     }
 
-    protected Map<InstanceIdentifier<?>, DataObject> fromDOMToData(
+    protected Map<InstanceIdentifier<?>, DataObject> toBinding(
             final Map<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, ? extends NormalizedNode<?, ?>> normalized) {
         Map<InstanceIdentifier<?>, DataObject> newMap = new HashMap<>();
         for (Map.Entry<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, ? extends NormalizedNode<?, ?>> entry : normalized
@@ -94,12 +99,38 @@ public abstract class AbstractForwardedDataBroker implements Delegator<DOMDataBr
                 Entry<InstanceIdentifier<? extends DataObject>, 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<InstanceIdentifier<?>> toBinding(
+            final Set<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier> normalized) {
+        Set<InstanceIdentifier<?>> hashSet = new HashSet<>();
+        for (org.opendaylight.yangtools.yang.data.api.InstanceIdentifier normalizedPath : normalized) {
+            try {
+                InstanceIdentifier<? extends DataObject> binding = getCodec().toBinding(normalizedPath);
+                hashSet.add(binding);
+            } catch (DeserializationException e) {
+                LOG.debug("Omitting {}", normalizedPath, e);
+            }
+        }
+        return hashSet;
+    }
+
+    protected Optional<DataObject> 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<DOMDataBr
         @Override
         public void onDataChanged(
                 final AsyncDataChangeEvent<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, NormalizedNode<?, ?>> change) {
-            bindingDataChangeListener.onDataChanged(new TranslatedDataChangeEvent(change,path));
+            bindingDataChangeListener.onDataChanged(new TranslatedDataChangeEvent(change, path));
         }
     }
 
     private class TranslatedDataChangeEvent implements AsyncDataChangeEvent<InstanceIdentifier<?>, DataObject> {
         private final AsyncDataChangeEvent<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, NormalizedNode<?, ?>> domEvent;
-        private InstanceIdentifier<?> path;
+        private final InstanceIdentifier<?> path;
 
-        public TranslatedDataChangeEvent(
-                final AsyncDataChangeEvent<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, NormalizedNode<?, ?>> change) {
-            this.domEvent = change;
-        }
+        private Map<InstanceIdentifier<?>, DataObject> createdCache;
+        private Map<InstanceIdentifier<?>, DataObject> updatedCache;
+        private Map<InstanceIdentifier<?>, ? extends DataObject> originalCache;
+        private Set<InstanceIdentifier<?>> removedCache;
+        private Optional<DataObject> originalDataCache;
+        private Optional<DataObject> updatedDataCache;
 
         public TranslatedDataChangeEvent(
                 final AsyncDataChangeEvent<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, NormalizedNode<?, ?>> change,
@@ -139,52 +172,63 @@ public abstract class AbstractForwardedDataBroker implements Delegator<DOMDataBr
 
         @Override
         public Map<InstanceIdentifier<?>, DataObject> getCreatedData() {
-            return fromDOMToData(domEvent.getCreatedData());
+            if (createdCache == null) {
+                createdCache = Collections.unmodifiableMap(toBinding(domEvent.getCreatedData()));
+            }
+            return createdCache;
         }
 
         @Override
         public Map<InstanceIdentifier<?>, DataObject> getUpdatedData() {
-            return fromDOMToData(domEvent.getUpdatedData());
+            if (updatedCache == null) {
+                updatedCache = Collections.unmodifiableMap(toBinding(domEvent.getUpdatedData()));
+            }
+            return updatedCache;
 
         }
 
         @Override
         public Set<InstanceIdentifier<?>> getRemovedPaths() {
-            final Set<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier> removedPaths = domEvent
-                    .getRemovedPaths();
-            final Set<InstanceIdentifier<?>> 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<InstanceIdentifier<?>, ? 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<DOMDataBr
         }
     }
 
-    protected DataObject toBindingData(final InstanceIdentifier<?> 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<DOMDataBr
 
     @Override
     public void setDomProviderContext(final ProviderSession domProviderContext) {
-       this.context = domProviderContext;
+        this.context = domProviderContext;
     }
 
     @Override
@@ -237,7 +272,4 @@ public abstract class AbstractForwardedDataBroker implements Delegator<DOMDataBr
         // NOOP
     }
 
-
-
-
 }

diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingToNormalizedNodeCodec.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingToNormalizedNodeCodec.java
index f885b323e6c04c34d23356c50722c24dfc755ac7..35ebe76499f51e6bb5b629c4cf42a944fe727ea8 100644 (file)
--- a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingToNormalizedNodeCodec.java
+++ b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingToNormalizedNodeCodec.java
@@ -10,6 +10,7 @@ package org.opendaylight.controller.md.sal.binding.impl;
 import java.util.AbstractMap.SimpleEntry;
 import java.util.Map.Entry;
 
+import org.opendaylight.controller.md.sal.common.impl.util.compat.DataNormalizationException;
 import org.opendaylight.controller.md.sal.common.impl.util.compat.DataNormalizer;
 import org.opendaylight.yangtools.yang.binding.Augmentation;
 import org.opendaylight.yangtools.yang.binding.DataObject;
@@ -43,7 +44,10 @@ public class BindingToNormalizedNodeCodec implements SchemaContextListener {
 
     public org.opendaylight.yangtools.yang.data.api.InstanceIdentifier toNormalized(
             final InstanceIdentifier<? extends DataObject> 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<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, NormalizedNode<?, ?>> toNormalizedNode(
@@ -54,7 +58,9 @@ public class BindingToNormalizedNodeCodec implements SchemaContextListener {
 
     public Entry<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, NormalizedNode<?, ?>> toNormalizedNode(
             final Entry<org.opendaylight.yangtools.yang.binding.InstanceIdentifier<? extends DataObject>, DataObject> binding) {
-        Entry<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, NormalizedNode<?, ?>> normalizedEntry = legacyToNormalized.toNormalized(bindingToLegacy.toDataDom(binding));
+        Entry<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, CompositeNode> legacyEntry = bindingToLegacy.toDataDom(binding);
+        Entry<org.opendaylight.yangtools.yang.data.api.InstanceIdentifier, NormalizedNode<?, ?>> normalizedEntry = legacyToNormalized.toNormalized(legacyEntry);
+        LOG.trace("Serialization of {}, Legacy Representation: {}, Normalized Representation: {}",binding,legacyEntry,normalizedEntry);
         if(Augmentation.class.isAssignableFrom(binding.getKey().getTargetType())) {
 
             for(DataContainerChild<? extends PathArgument, ?> 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 8cb4a70f9c2054495b965244436955e1eb67d3f3..ce1ff450c979775e2546d8c2463580abc7b57d5e 100644 (file)
--- 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<InstanceIdentifier<?>, DataObject> delegate;
+        private Map<InstanceIdentifier<?>, DataObject> updatedCache;
 
         public OperationalChangeEvent(final AsyncDataChangeEvent<InstanceIdentifier<?>, DataObject> change) {
             this.delegate = change;
@@ -128,7 +130,15 @@ public abstract class LegacyDataChangeEvent implements
 
         @Override
         public Map<InstanceIdentifier<?>, DataObject> getUpdatedOperationalData() {
-            return delegate.getUpdatedData();
+            if(updatedCache == null) {
+                Map<InstanceIdentifier<?>, DataObject> created = delegate.getCreatedData();
+                Map<InstanceIdentifier<?>, DataObject> updated = delegate.getUpdatedData();
+                HashMap<InstanceIdentifier<?>, 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<InstanceIdentifier<?>, DataObject> delegate;
+        private Map<InstanceIdentifier<?>, DataObject> updatedCache;
 
         public ConfigurationChangeEvent(final AsyncDataChangeEvent<InstanceIdentifier<?>, DataObject> change) {
             this.delegate = change;
@@ -174,7 +185,15 @@ public abstract class LegacyDataChangeEvent implements
 
         @Override
         public Map<InstanceIdentifier<?>, DataObject> getUpdatedConfigurationData() {
-            return delegate.getUpdatedData();
+            if(updatedCache == null) {
+                Map<InstanceIdentifier<?>, DataObject> created = delegate.getCreatedData();
+                Map<InstanceIdentifier<?>, DataObject> updated = delegate.getUpdatedData();
+                HashMap<InstanceIdentifier<?>, 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 e52e196326825ed362788934080c2b16597174f1..8fb6ff38a2477243545fe6fef86ca48532c4d493 100644 (file)
--- 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<PathArgument> 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());
     }