From 14d8f67545ed36f9fd4aad9444248c82aa7615bd Mon Sep 17 00:00:00 2001 From: Jakub Morvay Date: Tue, 2 May 2017 18:54:52 +0200 Subject: [PATCH] Fix DistributesShardedDOMDataTree.ProxyProducer's getShardAccess 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 --- .../DistributedShardedDOMDataTree.java | 32 ++++++++++++---- .../DistributedShardedDOMDataTreeTest.java | 37 +++++++++++++++++++ 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/sharding/DistributedShardedDOMDataTree.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/sharding/DistributedShardedDOMDataTree.java index 564fb978be..4ffc575c1a 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/sharding/DistributedShardedDOMDataTree.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/sharding/DistributedShardedDOMDataTree.java @@ -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 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> shardTable; + ProxyProducer(final DOMDataTreeProducer delegate, final Collection subtrees, final ActorRef shardDataTreeActor, - final ActorContext actorContext) { + final ActorContext actorContext, + final DOMDataTreePrefixTable> 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> 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; } } diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/sharding/DistributedShardedDOMDataTreeTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/sharding/DistributedShardedDOMDataTreeTest.java index 70ca5b5171..8faaf6ac0c 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/sharding/DistributedShardedDOMDataTreeTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/sharding/DistributedShardedDOMDataTreeTest.java @@ -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 createOuterEntries(final int amount, final String valuePrefix) { final Collection ret = new ArrayList<>(); for (int i = 0; i < amount; i++) { -- 2.36.6