Fix DistributesShardedDOMDataTree.ProxyProducer's getShardAccess 79/56579/1
authorJakub Morvay <jmorvay@cisco.com>
Tue, 2 May 2017 16:54:52 +0000 (18:54 +0200)
committerJakub Morvay <jmorvay@cisco.com>
Fri, 5 May 2017 08:42:13 +0000 (10:42 +0200)
DistributesShardedDOMDataTree.ProxyProducer's getShardAccess works only
for subtrees that are rooted at some registered prefix based shard.
Moreover subtree has to be one of the subtrees specified in
DistributedShardedDOMDatatTree's createProducer method.

This is way more strict than what is required by CDSDataTreeProducer's
API. Pass ProxyProducer's implementation current shard layout, so
producer can lookup corresponding shard for specified subtree in
getShardAccess method. One-to-one mapping between shards and subtrees
is no longer required.

Change-Id: I765567d34c803a85b4be8a6e10fd81b6f64a1610
Signed-off-by: Jakub Morvay <jmorvay@cisco.com>
opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/sharding/DistributedShardedDOMDataTree.java
opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/sharding/DistributedShardedDOMDataTreeTest.java

index 564fb97..4ffc575 100644 (file)
@@ -307,7 +307,7 @@ public class DistributedShardedDOMDataTree implements DOMDataTreeService, DOMDat
             LOG.debug("{} - Received success from remote nodes, creating producer:{}",
                     distributedConfigDatastore.getActorContext().getClusterWrapper().getCurrentMemberName(), subtrees);
             return new ProxyProducer(producer, subtrees, shardedDataTreeActor,
-                    distributedConfigDatastore.getActorContext());
+                    distributedConfigDatastore.getActorContext(), shards);
         } else if (response instanceof Exception) {
             closeProducer(producer);
             throw Throwables.propagate((Exception) response);
@@ -642,14 +642,21 @@ public class DistributedShardedDOMDataTree implements DOMDataTreeService, DOMDat
         @GuardedBy("shardAccessMap")
         private final Map<DOMDataTreeIdentifier, CDSShardAccessImpl> shardAccessMap = new HashMap<>();
 
+        // We don't have to guard access to shardTable in ProxyProducer.
+        // ShardTable's entries relevant to this ProxyProducer shouldn't
+        // change during producer's lifetime.
+        private final DOMDataTreePrefixTable<DOMDataTreeShardRegistration<DOMDataTreeShard>> shardTable;
+
         ProxyProducer(final DOMDataTreeProducer delegate,
                       final Collection<DOMDataTreeIdentifier> subtrees,
                       final ActorRef shardDataTreeActor,
-                      final ActorContext actorContext) {
+                      final ActorContext actorContext,
+                      final DOMDataTreePrefixTable<DOMDataTreeShardRegistration<DOMDataTreeShard>> shardLayout) {
             this.delegate = Preconditions.checkNotNull(delegate);
             this.subtrees = Preconditions.checkNotNull(subtrees);
             this.shardDataTreeActor = Preconditions.checkNotNull(shardDataTreeActor);
             this.actorContext = Preconditions.checkNotNull(actorContext);
+            this.shardTable = Preconditions.checkNotNull(shardLayout);
         }
 
         @Nonnull
@@ -670,6 +677,7 @@ public class DistributedShardedDOMDataTree implements DOMDataTreeService, DOMDat
         @SuppressWarnings("checkstyle:IllegalCatch")
         public void close() throws DOMDataTreeProducerException {
             delegate.close();
+
             synchronized (shardAccessMap) {
                 shardAccessMap.values().forEach(CDSShardAccessImpl::close);
             }
@@ -690,19 +698,27 @@ public class DistributedShardedDOMDataTree implements DOMDataTreeService, DOMDat
         @Nonnull
         @Override
         public CDSShardAccess getShardAccess(@Nonnull final DOMDataTreeIdentifier subtree) {
+            Preconditions.checkArgument(
+                    subtrees.stream().anyMatch(dataTreeIdentifier -> dataTreeIdentifier.contains(subtree)),
+                    "Subtree {} is not controlled by this producer {}", subtree, this);
+
+            final DOMDataTreePrefixTableEntry<DOMDataTreeShardRegistration<DOMDataTreeShard>> lookup =
+                    shardTable.lookup(subtree);
+            Preconditions.checkState(lookup != null, "Subtree {} is not contained in any registered shard.");
+
+            final DOMDataTreeIdentifier lookupId = lookup.getValue().getPrefix();
+
             synchronized (shardAccessMap) {
-                Preconditions.checkArgument(subtrees.contains(subtree),
-                        "Subtree {} is not controlled by this producer {}", subtree, this);
-                if (shardAccessMap.get(subtree) != null) {
-                    return shardAccessMap.get(subtree);
+                if (shardAccessMap.get(lookupId) != null) {
+                    return shardAccessMap.get(lookupId);
                 }
 
                 // TODO Maybe we can have static factory method and return the same instance
                 // for same subtrees. But maybe it is not needed since there can be only one
                 // producer attached to some subtree at a time. And also how we can close ShardAccess
                 // then
-                final CDSShardAccessImpl shardAccess = new CDSShardAccessImpl(subtree, actorContext);
-                shardAccessMap.put(subtree, shardAccess);
+                final CDSShardAccessImpl shardAccess = new CDSShardAccessImpl(lookupId, actorContext);
+                shardAccessMap.put(lookupId, shardAccess);
                 return shardAccess;
             }
         }
index 70ca5b5..8faaf6a 100644 (file)
@@ -10,6 +10,7 @@ package org.opendaylight.controller.cluster.sharding;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.anyCollection;
 import static org.mockito.Matchers.anyMap;
 import static org.mockito.Mockito.doNothing;
@@ -55,6 +56,8 @@ import org.opendaylight.controller.cluster.datastore.DatastoreContext.Builder;
 import org.opendaylight.controller.cluster.datastore.DistributedDataStore;
 import org.opendaylight.controller.cluster.datastore.IntegrationTestKit;
 import org.opendaylight.controller.cluster.datastore.utils.ClusterUtils;
+import org.opendaylight.controller.cluster.dom.api.CDSDataTreeProducer;
+import org.opendaylight.controller.cluster.dom.api.CDSShardAccess;
 import org.opendaylight.controller.cluster.raft.utils.InMemoryJournal;
 import org.opendaylight.controller.cluster.raft.utils.InMemorySnapshotStore;
 import org.opendaylight.controller.cluster.sharding.DistributedShardFactory.DistributedShardRegistration;
@@ -447,6 +450,40 @@ public class DistributedShardedDOMDataTreeTest extends AbstractTest {
         }
     }
 
+    @Test
+    public void testCDSDataTreeProducer() throws Exception {
+        initEmptyDatastores();
+
+        final DistributedShardRegistration reg1 = waitOnAsyncTask(leaderShardFactory.createDistributedShard(
+                TEST_ID, Lists.newArrayList(AbstractTest.MEMBER_NAME)),
+                DistributedShardedDOMDataTree.SHARD_FUTURE_TIMEOUT_DURATION);
+
+        leaderTestKit.waitUntilLeader(leaderDistributedDataStore.getActorContext(),
+                ClusterUtils.getCleanShardName(TestModel.TEST_PATH));
+
+        assertNotNull(findLocalShard(leaderDistributedDataStore.getActorContext(),
+                ClusterUtils.getCleanShardName(TestModel.TEST_PATH)));
+
+
+        final DOMDataTreeIdentifier configRoot =
+                new DOMDataTreeIdentifier(LogicalDatastoreType.CONFIGURATION, YangInstanceIdentifier.EMPTY);
+        final DOMDataTreeProducer producer = leaderShardFactory.createProducer(Collections.singleton(configRoot));
+
+        assertTrue(producer instanceof CDSDataTreeProducer);
+
+        final CDSDataTreeProducer cdsProducer = (CDSDataTreeProducer) producer;
+        CDSShardAccess shardAccess = cdsProducer.getShardAccess(TEST_ID);
+        assertEquals(shardAccess.getShardIdentifier(), TEST_ID);
+
+        shardAccess = cdsProducer.getShardAccess(INNER_LIST_ID);
+        assertEquals(TEST_ID, shardAccess.getShardIdentifier());
+
+        shardAccess = cdsProducer.getShardAccess(configRoot);
+        assertEquals(configRoot, shardAccess.getShardIdentifier());
+
+        waitOnAsyncTask(reg1.close(), DistributedShardedDOMDataTree.SHARD_FUTURE_TIMEOUT_DURATION);
+    }
+
     private static Collection<MapEntryNode> createOuterEntries(final int amount, final String valuePrefix) {
         final Collection<MapEntryNode> ret = new ArrayList<>();
         for (int i = 0; i < amount; i++) {

©2013 OpenDaylight, A Linux Foundation Collaborative Project. All Rights Reserved.
OpenDaylight is a registered trademark of The OpenDaylight Project, Inc.
Linux Foundation and OpenDaylight are registered trademarks of the Linux Foundation.
Linux is a registered trademark of Linus Torvalds.