Merge "BUG-704 Fix failing integration test in netconf-it"
authorTony Tkacik <ttkacik@cisco.com>
Wed, 9 Apr 2014 08:19:07 +0000 (08:19 +0000)
committerGerrit Code Review <gerrit@opendaylight.org>
Wed, 9 Apr 2014 08:19:07 +0000 (08:19 +0000)
opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/DataChangeEventResolver.java
opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/InMemoryDOMDataStore.java
opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/SchemaAwareApplyOperation.java
opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/ListenerRegistrationNode.java
opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/StoreMetadataNode.java
opendaylight/md-sal/sal-dom-broker/src/test/java/org/opendaylight/controller/md/sal/dom/store/impl/ModificationMetadataTreeTest.java

index 0948f6d3e56f9bd285240c39dc9a6b2f80039b6f..f231bb5c39e32c3cfb6b998873d0fe3d748ecdc3 100644 (file)
@@ -4,8 +4,9 @@ import static org.opendaylight.controller.md.sal.dom.store.impl.DOMImmutableData
 import static org.opendaylight.controller.md.sal.dom.store.impl.StoreUtils.append;
 import static org.opendaylight.controller.md.sal.dom.store.impl.tree.TreeNodeUtils.getChild;
 
-import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
+import java.util.Set;
 
 import org.opendaylight.controller.md.sal.common.api.data.AsyncDataBroker.DataChangeScope;
 import org.opendaylight.controller.md.sal.dom.store.impl.DOMImmutableDataChangeEvent.Builder;
@@ -23,17 +24,14 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 
 public class DataChangeEventResolver {
-
-
-    private  static final Logger LOG = LoggerFactory.getLogger(DataChangeEventResolver.class);
-
+    private static final Logger LOG = LoggerFactory.getLogger(DataChangeEventResolver.class);
     private static final DOMImmutableDataChangeEvent NO_CHANGE = builder().build();
+    private final ImmutableList.Builder<ChangeListenerNotifyTask> tasks = ImmutableList.builder();
     private InstanceIdentifier rootPath;
     private ListenerRegistrationNode listenerRoot;
     private NodeModification modificationRoot;
     private Optional<StoreMetadataNode> beforeRoot;
     private Optional<StoreMetadataNode> afterRoot;
-    private final ImmutableList.Builder<ChangeListenerNotifyTask> tasks = ImmutableList.builder();
 
     protected InstanceIdentifier getRootPath() {
         return rootPath;
@@ -133,11 +131,7 @@ public class DataChangeEventResolver {
             builder.merge(resolveCreateEvent(childPath, childListeners, child));
         }
 
-        DOMImmutableDataChangeEvent event = builder.build();
-        if (listeners.isPresent()) {
-            addNotifyTask(listeners.get().getListeners(), event);
-        }
-        return event;
+        return addNotifyTask(listeners, builder.build());
     }
 
     private DOMImmutableDataChangeEvent resolveDeleteEvent(final InstanceIdentifier path,
@@ -151,12 +145,7 @@ public class DataChangeEventResolver {
             InstanceIdentifier childPath = StoreUtils.append(path, childId);
             builder.merge(resolveDeleteEvent(childPath, childListeners, child));
         }
-        DOMImmutableDataChangeEvent event = builder.build();
-        if (listeners.isPresent()) {
-            addNotifyTask(listeners.get().getListeners(), event);
-        }
-        return event;
-
+        return addNotifyTask(listeners, builder.build());
     }
 
     private DOMImmutableDataChangeEvent resolveSubtreeChangeEvent(final InstanceIdentifier path,
@@ -206,33 +195,35 @@ public class DataChangeEventResolver {
         return builder().build();
     }
 
-    private void addNotifyTask(final ListenerRegistrationNode listenerRegistrationNode, final DataChangeScope scope,
-            final DOMImmutableDataChangeEvent event) {
-        Collection<DataChangeListenerRegistration<?>> potential = listenerRegistrationNode.getListeners();
-        if(potential.isEmpty()) {
-            return;
-        }
-        ArrayList<DataChangeListenerRegistration<?>> toNotify = new ArrayList<>(potential.size());
-        for(DataChangeListenerRegistration<?> listener : potential) {
-            if(scope.equals(listener.getScope())) {
-                toNotify.add(listener);
+    private DOMImmutableDataChangeEvent addNotifyTask(final Optional<ListenerRegistrationNode> listeners, final DOMImmutableDataChangeEvent event) {
+        if (listeners.isPresent()) {
+            final Collection<DataChangeListenerRegistration<?>> l = listeners.get().getListeners();
+            if (!l.isEmpty()) {
+                tasks.add(new ChangeListenerNotifyTask(ImmutableSet.copyOf(l), event));
             }
         }
-        addNotifyTask(toNotify, event);
 
+        return event;
     }
 
-    private void addNotifyTask(final Collection<DataChangeListenerRegistration<?>> listeners,
+    private void addNotifyTask(final ListenerRegistrationNode listenerRegistrationNode, final DataChangeScope scope,
             final DOMImmutableDataChangeEvent event) {
-        if(!listeners.isEmpty()) {
-            tasks.add(new ChangeListenerNotifyTask(ImmutableSet.copyOf(listeners),event));
+        Collection<DataChangeListenerRegistration<?>> potential = listenerRegistrationNode.getListeners();
+        if(!potential.isEmpty()) {
+            final Set<DataChangeListenerRegistration<?>> toNotify = new HashSet<>(potential.size());
+            for(DataChangeListenerRegistration<?> listener : potential) {
+                if(scope.equals(listener.getScope())) {
+                    toNotify.add(listener);
+                }
+            }
+
+            if (!toNotify.isEmpty()) {
+                tasks.add(new ChangeListenerNotifyTask(toNotify, event));
+            }
         }
     }
 
     public static DataChangeEventResolver create() {
         return new DataChangeEventResolver();
     }
-
-
-
 }
index 2091913f24949106aae128e07083a4354b992b70..de0c1463923ebe0e2600a79191e590cf7552376f 100644 (file)
@@ -27,7 +27,9 @@ import org.opendaylight.controller.sal.core.spi.data.DOMStore;
 import org.opendaylight.controller.sal.core.spi.data.DOMStoreReadTransaction;
 import org.opendaylight.controller.sal.core.spi.data.DOMStoreReadWriteTransaction;
 import org.opendaylight.controller.sal.core.spi.data.DOMStoreThreePhaseCommitCohort;
+import org.opendaylight.controller.sal.core.spi.data.DOMStoreTransaction;
 import org.opendaylight.controller.sal.core.spi.data.DOMStoreWriteTransaction;
+import org.opendaylight.yangtools.concepts.AbstractListenerRegistration;
 import org.opendaylight.yangtools.concepts.Identifiable;
 import org.opendaylight.yangtools.concepts.ListenerRegistration;
 import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier;
@@ -39,6 +41,8 @@ import org.opendaylight.yangtools.yang.model.api.SchemaContextListener;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import com.google.common.base.Objects;
+import com.google.common.base.Objects.ToStringHelper;
 import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
 import com.google.common.primitives.UnsignedLong;
@@ -99,11 +103,6 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
     @Override
     public <L extends AsyncDataChangeListener<InstanceIdentifier, NormalizedNode<?, ?>>> ListenerRegistration<L> registerChangeListener(
             final InstanceIdentifier path, final L listener, final DataChangeScope scope) {
-        LOG.debug("{}: Registering data change listener {} for {}",name,listener,path);
-        ListenerRegistrationNode listenerNode = listenerTree;
-        for(PathArgument arg : path.getPath()) {
-            listenerNode = listenerNode.ensureChild(arg);
-        }
 
         /*
          * Make sure commit is not occurring right now. Listener has to be registered and its
@@ -114,6 +113,12 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
          */
         final DataChangeListenerRegistration<L> reg;
         synchronized (this) {
+            LOG.debug("{}: Registering data change listener {} for {}",name,listener,path);
+            ListenerRegistrationNode listenerNode = listenerTree;
+            for(PathArgument arg : path.getPath()) {
+                listenerNode = listenerNode.ensureChild(arg);
+            }
+
             reg = listenerNode.registerDataChangeListener(path, listener, scope);
 
             Optional<StoreMetadataNode> currentState = snapshot.get().read(path);
@@ -128,7 +133,14 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
             }
         }
 
-        return reg;
+        return new AbstractListenerRegistration<L>(listener) {
+            @Override
+            protected void removeRegistration() {
+                synchronized (InMemoryDOMDataStore.this) {
+                    reg.close();
+                }
+            }
+        };
     }
 
     private synchronized DOMStoreThreePhaseCommitCohort submit(
@@ -142,7 +154,7 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
     }
 
     private void commit(final DataAndMetadataSnapshot currentSnapshot,
-            final StoreMetadataNode newDataTree, final Iterable<ChangeListenerNotifyTask> listenerTasks) {
+            final StoreMetadataNode newDataTree, final DataChangeEventResolver listenerResolver) {
         LOG.debug("Updating Store snaphot version: {} with version:{}",currentSnapshot.getMetadataTree().getSubtreeVersion(),newDataTree.getSubtreeVersion());
 
         if(LOG.isTraceEnabled()) {
@@ -161,31 +173,52 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
             final boolean success = snapshot.compareAndSet(currentSnapshot, newSnapshot);
             checkState(success, "Store snapshot and transaction snapshot differ. This should never happen.");
 
-            for (ChangeListenerNotifyTask task : listenerTasks) {
+            for (ChangeListenerNotifyTask task : listenerResolver.resolve()) {
                 executor.submit(task);
             }
         }
     }
 
-    private static class SnapshotBackedReadTransaction implements DOMStoreReadTransaction {
-
-        private DataAndMetadataSnapshot stableSnapshot;
+    private static abstract class AbstractDOMStoreTransaction implements DOMStoreTransaction {
         private final Object identifier;
 
-        public SnapshotBackedReadTransaction(final Object identifier, final DataAndMetadataSnapshot snapshot) {
+        protected AbstractDOMStoreTransaction(final Object identifier) {
             this.identifier = identifier;
-            this.stableSnapshot = snapshot;
-            LOG.debug("ReadOnly Tx: {} allocated with snapshot {}",identifier,snapshot.getMetadataTree().getSubtreeVersion());
-
         }
 
         @Override
-        public Object getIdentifier() {
+        public final Object getIdentifier() {
             return identifier;
         }
 
+        @Override
+        public final String toString() {
+            return addToStringAttributes(Objects.toStringHelper(this)).toString();
+        }
+
+        /**
+         * Add class-specific toString attributes.
+         *
+         * @param toStringHelper ToStringHelper instance
+         * @return ToStringHelper instance which was passed in
+         */
+        protected ToStringHelper addToStringAttributes(final ToStringHelper toStringHelper) {
+            return toStringHelper.add("id", identifier);
+        }
+    }
+
+    private static class SnapshotBackedReadTransaction extends AbstractDOMStoreTransaction implements DOMStoreReadTransaction {
+        private DataAndMetadataSnapshot stableSnapshot;
+
+        public SnapshotBackedReadTransaction(final Object identifier, final DataAndMetadataSnapshot snapshot) {
+            super(identifier);
+            this.stableSnapshot = Preconditions.checkNotNull(snapshot);
+            LOG.debug("ReadOnly Tx: {} allocated with snapshot {}", identifier, snapshot.getMetadataTree().getSubtreeVersion());
+        }
+
         @Override
         public void close() {
+            LOG.debug("Store transaction: {} : Closed", getIdentifier());
             stableSnapshot = null;
         }
 
@@ -195,37 +228,24 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
             checkState(stableSnapshot != null, "Transaction is closed");
             return Futures.immediateFuture(NormalizedNodeUtils.findNode(stableSnapshot.getDataTree(), path));
         }
-
-        @Override
-        public String toString() {
-            return "SnapshotBackedReadTransaction [id =" + identifier + "]";
-        }
-
     }
 
-    private static class SnaphostBackedWriteTransaction implements DOMStoreWriteTransaction {
-
+    private static class SnaphostBackedWriteTransaction extends AbstractDOMStoreTransaction implements DOMStoreWriteTransaction {
         private MutableDataTree mutableTree;
-        private final Object identifier;
         private InMemoryDOMDataStore store;
-
         private boolean ready = false;
 
         public SnaphostBackedWriteTransaction(final Object identifier, final DataAndMetadataSnapshot snapshot,
                 final InMemoryDOMDataStore store, final ModificationApplyOperation applyOper) {
-            this.identifier = identifier;
+            super(identifier);
             mutableTree = MutableDataTree.from(snapshot, applyOper);
             this.store = store;
             LOG.debug("Write Tx: {} allocated with snapshot {}",identifier,snapshot.getMetadataTree().getSubtreeVersion());
         }
 
-        @Override
-        public Object getIdentifier() {
-            return identifier;
-        }
-
         @Override
         public void close() {
+            LOG.debug("Store transaction: {} : Closed", getIdentifier());
             this.mutableTree = null;
             this.store = null;
         }
@@ -242,17 +262,19 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
             mutableTree.delete(path);
         }
 
-        protected boolean isReady() {
+        protected final boolean isReady() {
             return ready;
         }
 
-        protected void checkNotReady() {
-            checkState(!ready, "Transaction is ready. No further modifications allowed.");
+        protected final void checkNotReady() {
+            checkState(!ready, "Transaction %s is ready. No further modifications allowed.", getIdentifier());
         }
 
         @Override
         public synchronized DOMStoreThreePhaseCommitCohort ready() {
+            checkState(!ready, "Transaction %s is already ready.", getIdentifier());
             ready = true;
+
             LOG.debug("Store transaction: {} : Ready", getIdentifier());
             mutableTree.seal();
             return store.submit(this);
@@ -263,10 +285,9 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
         }
 
         @Override
-        public String toString() {
-            return "SnaphostBackedWriteTransaction [id=" + getIdentifier() + ", ready=" + isReady() + "]";
+        protected ToStringHelper addToStringAttributes(final ToStringHelper toStringHelper) {
+            return toStringHelper.add("ready", isReady());
         }
-
     }
 
     private static class SnapshotBackedReadWriteTransaction extends SnaphostBackedWriteTransaction implements
@@ -281,12 +302,6 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
         public ListenableFuture<Optional<NormalizedNode<?, ?>>> read(final InstanceIdentifier path) {
             return Futures.immediateFuture(getMutatedView().read(path));
         }
-
-        @Override
-        public String toString() {
-            return "SnapshotBackedReadWriteTransaction [id=" + getIdentifier() + ", ready=" + isReady() + "]";
-        }
-
     }
 
     private class ThreePhaseCommitImpl implements DOMStoreThreePhaseCommitCohort {
@@ -296,7 +311,7 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
 
         private DataAndMetadataSnapshot storeSnapshot;
         private Optional<StoreMetadataNode> proposedSubtree;
-        private Iterable<ChangeListenerNotifyTask> listenerTasks;
+        private DataChangeEventResolver listenerResolver;
 
         public ThreePhaseCommitImpl(final SnaphostBackedWriteTransaction writeTransaction) {
             this.transaction = writeTransaction;
@@ -337,13 +352,12 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
                     proposedSubtree = operationTree.apply(modification, Optional.of(metadataTree),
                             increase(metadataTree.getSubtreeVersion()));
 
-                    listenerTasks = DataChangeEventResolver.create() //
+                    listenerResolver = DataChangeEventResolver.create() //
                             .setRootPath(PUBLIC_ROOT_PATH) //
                             .setBeforeRoot(Optional.of(metadataTree)) //
                             .setAfterRoot(proposedSubtree) //
                             .setModificationRoot(modification) //
-                            .setListenerRoot(listenerTree) //
-                            .resolve();
+                            .setListenerRoot(listenerTree);
 
                     return null;
                 }
@@ -366,7 +380,7 @@ public class InMemoryDOMDataStore implements DOMStore, Identifiable<String>, Sch
             checkState(proposedSubtree != null,"Proposed subtree must be computed");
             checkState(storeSnapshot != null,"Proposed subtree must be computed");
             // return ImmediateFuture<>;
-            InMemoryDOMDataStore.this.commit(storeSnapshot, proposedSubtree.get(),listenerTasks);
+            InMemoryDOMDataStore.this.commit(storeSnapshot, proposedSubtree.get(),listenerResolver);
             return Futures.<Void> immediateFuture(null);
         }
 
index fd8560773ba10ff74b5130eec297be3d3f6e9c53..00727347eea95f7378ebc53fc9cf97d3cc7fc7f5 100644 (file)
@@ -275,7 +275,7 @@ public abstract class SchemaAwareApplyOperation implements ModificationApplyOper
             if (currentMeta.isPresent()) {
                 nodeVersion = StoreUtils.increase(currentMeta.get().getNodeVersion());
             }
-            StoreMetadataNode newValueMeta = StoreMetadataNode.createRecursivelly(newValue, nodeVersion, nodeVersion);
+            StoreMetadataNode newValueMeta = StoreMetadataNode.createRecursively(newValue, nodeVersion, nodeVersion);
 
             if (!modification.hasAdditionalModifications()) {
                 return newValueMeta;
index 2528d383b95a0c9f6534ad0f01627f56dbc26c5c..854c125af17a732f1d1c40365cb355ddae25a317 100644 (file)
@@ -46,9 +46,16 @@ public class ListenerRegistrationNode implements StoreTreeNode<ListenerRegistrat
         return identifier;
     }
 
+    /**
+     * Return the list of current listeners. Any caller wishing to use this method
+     * has to make sure the collection remains unchanged while it's executing. This
+     * means the caller has to synchronize externally both the registration and
+     * unregistration process.
+     *
+     * @return the list of current listeners
+     */
     @SuppressWarnings({ "rawtypes", "unchecked" })
     public Collection<org.opendaylight.controller.md.sal.dom.store.impl.DataChangeListenerRegistration<?>> getListeners() {
-        // FIXME: this is not thread-safe and races with listener (un)registration!
         return (Collection) listeners;
     }
 
@@ -67,7 +74,6 @@ public class ListenerRegistrationNode implements StoreTreeNode<ListenerRegistrat
     }
 
     /**
-     *
      * Registers listener on this node.
      *
      * @param path Full path on which listener is registered.
@@ -75,16 +81,18 @@ public class ListenerRegistrationNode implements StoreTreeNode<ListenerRegistrat
      * @param scope Scope of triggering event.
      * @return
      */
-    public <L extends AsyncDataChangeListener<InstanceIdentifier, NormalizedNode<?, ?>>> DataChangeListenerRegistration<L> registerDataChangeListener(final InstanceIdentifier path,
+    public synchronized <L extends AsyncDataChangeListener<InstanceIdentifier, NormalizedNode<?, ?>>> DataChangeListenerRegistration<L> registerDataChangeListener(final InstanceIdentifier path,
             final L listener, final DataChangeScope scope) {
 
         DataChangeListenerRegistration<L> listenerReg = new DataChangeListenerRegistration<L>(path,listener, scope, this);
         listeners.add(listenerReg);
+        LOG.debug("Listener {} registered", listener);
         return listenerReg;
     }
 
-    private void removeListener(final DataChangeListenerRegistration<?> listener) {
+    private synchronized void removeListener(final DataChangeListenerRegistration<?> listener) {
         listeners.remove(listener);
+        LOG.debug("Listener {} unregistered", listener);
         removeThisIfUnused();
     }
 
index ea83047a3101996e891a033f49f0fe34eee13990..d42f36fb90768757bcda5f4878f7ef985b827af4 100644 (file)
@@ -96,7 +96,7 @@ public class StoreMetadataNode implements Immutable, Identifiable<PathArgument>,
         return Optional.absent();
     }
 
-    public static final StoreMetadataNode createRecursivelly(final NormalizedNode<?, ?> node,
+    public static final StoreMetadataNode createRecursively(final NormalizedNode<?, ?> node,
             final UnsignedLong nodeVersion, final UnsignedLong subtreeVersion) {
         Builder builder = builder() //
                 .setNodeVersion(nodeVersion) //
@@ -107,7 +107,7 @@ public class StoreMetadataNode implements Immutable, Identifiable<PathArgument>,
             @SuppressWarnings("unchecked")
             NormalizedNodeContainer<?, ?, NormalizedNode<?, ?>> nodeContainer = (NormalizedNodeContainer<?, ?, NormalizedNode<?, ?>>) node;
             for (NormalizedNode<?, ?> subNode : nodeContainer.getValue()) {
-                builder.add(createRecursivelly(subNode, nodeVersion, subtreeVersion));
+                builder.add(createRecursively(subNode, nodeVersion, subtreeVersion));
             }
         }
         return builder.build();
@@ -162,8 +162,8 @@ public class StoreMetadataNode implements Immutable, Identifiable<PathArgument>,
         }
     }
 
-    public static StoreMetadataNode createRecursivelly(final NormalizedNode<?, ?> node, final UnsignedLong version) {
-        return createRecursivelly(node, version, version);
+    public static StoreMetadataNode createRecursively(final NormalizedNode<?, ?> node, final UnsignedLong version) {
+        return createRecursively(node, version, version);
     }
 
 }
index 7adc3c713b2b28fc1c8c40f286cb74f8ab3ef5f2..e14699311bd33da365e923c6d76a66c4c6d17009 100644 (file)
@@ -139,7 +139,7 @@ public class ModificationMetadataTreeTest {
     public void basicReadWrites() {
         MutableDataTree modificationTree = MutableDataTree.from(
                 DataAndMetadataSnapshot.builder() //
-                        .setMetadataTree(StoreMetadataNode.createRecursivelly(createDocumentOne(), UnsignedLong.valueOf(5))) //
+                        .setMetadataTree(StoreMetadataNode.createRecursively(createDocumentOne(), UnsignedLong.valueOf(5))) //
                         .setSchemaContext(schemaContext) //
                         .build(), new SchemaAwareApplyOperationRoot(schemaContext));
         Optional<NormalizedNode<?, ?>> originalBarNode = modificationTree.read(OUTER_LIST_2_PATH);