From 0c2d8a6530db21b1ca89aff8ee3812aa801cddac Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Thu, 31 Aug 2017 22:48:04 +0200 Subject: [PATCH] Separate ShardRecoveryCoordinator implementations This patch turns ShardRecoveryCoordinator into two concrete implementations based on presence of snapshot. This will allow us to catch cases where restore from snapshot gives us a null snapshot. Furthermore it cleans up the test a bit, byu sharing coordination instantiation code and using assertTrue/assertFalse to prevent boxing operations implied by assertEquals. Change-Id: I8e4ceba0ec6df70e5557f6f457c60ed2cd39a8bd Signed-off-by: Robert Varga --- .../controller/cluster/datastore/Shard.java | 7 ++- .../datastore/ShardRecoveryCoordinator.java | 45 ++++++++++++++----- .../ShardRecoveryCoordinatorTest.java | 23 ++++------ 3 files changed, 49 insertions(+), 26 deletions(-) diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/Shard.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/Shard.java index 1c85aef978..1789f09d2b 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/Shard.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/Shard.java @@ -839,8 +839,11 @@ public class Shard extends RaftActor { @Override @Nonnull protected RaftActorRecoveryCohort getRaftActorRecoveryCohort() { - return new ShardRecoveryCoordinator(store, - restoreFromSnapshot != null ? restoreFromSnapshot.getSnapshot() : null, persistenceId(), LOG); + if (restoreFromSnapshot == null) { + return ShardRecoveryCoordinator.create(store, persistenceId(), LOG); + } + + return ShardRecoveryCoordinator.forSnapshot(store, persistenceId(), LOG, restoreFromSnapshot.getSnapshot()); } @Override diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardRecoveryCoordinator.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardRecoveryCoordinator.java index 6e5886132a..5fc3c7adc2 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardRecoveryCoordinator.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardRecoveryCoordinator.java @@ -27,21 +27,51 @@ import org.slf4j.Logger; * * @author Thomas Pantelis */ -class ShardRecoveryCoordinator implements RaftActorRecoveryCohort { +abstract class ShardRecoveryCoordinator implements RaftActorRecoveryCohort { + private static final class Simple extends ShardRecoveryCoordinator { + Simple(final ShardDataTree store, final String shardName, final Logger log) { + super(store, shardName, log); + } + + @Override + public Snapshot getRestoreFromSnapshot() { + return null; + } + } + + private static final class WithSnapshot extends ShardRecoveryCoordinator { + private final Snapshot restoreFromSnapshot; + + WithSnapshot(final ShardDataTree store, final String shardName, final Logger log, final Snapshot snapshot) { + super(store, shardName, log); + this.restoreFromSnapshot = Preconditions.checkNotNull(snapshot); + } + + @Override + public Snapshot getRestoreFromSnapshot() { + return restoreFromSnapshot; + } + } + private final ShardDataTree store; private final String shardName; private final Logger log; - private final Snapshot restoreFromSnapshot; private boolean open; - ShardRecoveryCoordinator(final ShardDataTree store, final Snapshot restoreFromSnapshot, final String shardName, - final Logger log) { + ShardRecoveryCoordinator(final ShardDataTree store, final String shardName, final Logger log) { this.store = Preconditions.checkNotNull(store); this.shardName = Preconditions.checkNotNull(shardName); this.log = Preconditions.checkNotNull(log); + } + + static ShardRecoveryCoordinator create(final ShardDataTree store, final String shardName, final Logger log) { + return new Simple(store, shardName, log); + } - this.restoreFromSnapshot = restoreFromSnapshot; + static ShardRecoveryCoordinator forSnapshot(final ShardDataTree store, final String shardName, final Logger log, + final Snapshot snapshot) { + return new WithSnapshot(store, shardName, log, snapshot); } @Override @@ -104,9 +134,4 @@ class ShardRecoveryCoordinator implements RaftActorRecoveryCohort { shardName, shardSnapshot, f), e); } } - - @Override - public Snapshot getRestoreFromSnapshot() { - return restoreFromSnapshot; - } } diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardRecoveryCoordinatorTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardRecoveryCoordinatorTest.java index fc257b8cbb..4467817a21 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardRecoveryCoordinatorTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardRecoveryCoordinatorTest.java @@ -8,7 +8,8 @@ package org.opendaylight.controller.cluster.datastore; -import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import com.google.common.base.Optional; @@ -32,13 +33,16 @@ import org.opendaylight.yangtools.yang.data.api.schema.tree.TreeType; import org.opendaylight.yangtools.yang.data.impl.schema.tree.InMemoryDataTreeFactory; import org.opendaylight.yangtools.yang.data.impl.schema.tree.SchemaValidationFailedException; import org.opendaylight.yangtools.yang.model.api.SchemaContext; +import org.slf4j.Logger; import org.slf4j.LoggerFactory; public class ShardRecoveryCoordinatorTest extends AbstractTest { + private static final Logger FOO_LOGGER = LoggerFactory.getLogger("foo"); private ShardDataTree peopleDataTree; private SchemaContext peopleSchemaContext; private SchemaContext carsSchemaContext; + private ShardRecoveryCoordinator coordinator; @Before public void setUp() { @@ -48,13 +52,12 @@ public class ShardRecoveryCoordinatorTest extends AbstractTest { final Shard mockShard = Mockito.mock(Shard.class); peopleDataTree = new ShardDataTree(mockShard, peopleSchemaContext, TreeType.OPERATIONAL); + coordinator = ShardRecoveryCoordinator.create(peopleDataTree, "foobar", FOO_LOGGER); + coordinator.startLogRecoveryBatch(10); } @Test public void testAppendRecoveredLogEntryCommitTransactionPayload() throws IOException { - final ShardRecoveryCoordinator coordinator = new ShardRecoveryCoordinator(peopleDataTree, - null, "foobar", LoggerFactory.getLogger("foo")); - coordinator.startLogRecoveryBatch(10); try { coordinator.appendRecoveredLogEntry(CommitTransactionPayload.create(nextTransactionId(), createCar())); } catch (final SchemaValidationFailedException e) { @@ -66,23 +69,15 @@ public class ShardRecoveryCoordinatorTest extends AbstractTest { @Test public void testApplyRecoverySnapshot() { - final ShardRecoveryCoordinator coordinator = new ShardRecoveryCoordinator(peopleDataTree, - null, "foobar", LoggerFactory.getLogger("foo")); - coordinator.startLogRecoveryBatch(10); - coordinator.applyRecoverySnapshot(createSnapshot()); - assertEquals(false, readCars(peopleDataTree).isPresent()); - assertEquals(true, readPeople(peopleDataTree).isPresent()); + assertFalse(readCars(peopleDataTree).isPresent()); + assertTrue(readPeople(peopleDataTree).isPresent()); } @Test public void testApplyCurrentLogRecoveryBatch() { - final ShardRecoveryCoordinator coordinator = new ShardRecoveryCoordinator(peopleDataTree, - null, "foobar", LoggerFactory.getLogger("foo")); - coordinator.startLogRecoveryBatch(10); - try { coordinator.applyCurrentLogRecoveryBatch(); } catch (final IllegalArgumentException e) { -- 2.36.6