From ff29db5dc6012f77bbe53f57ddce929b0de093b3 Mon Sep 17 00:00:00 2001 From: "tadei.bilan" Date: Mon, 27 Jul 2020 13:18:29 +0300 Subject: [PATCH] Do not reset datasize after a fake snapshot If we reset dataSize from SnapshotManager's fake snapshot, we would not have correctly accounted for on-disk size. Fix this by exposing a simple knob, which allows suppressing dataSize update during snapshotCommit(). JIRA: CONTROLLER-1957 Change-Id: I553ddb523ac8504892cc5353bfe4b002c25436ce Signed-off-by: tadei.bilan Signed-off-by: Robert Varga --- .../raft/AbstractReplicatedLogImpl.java | 16 +++++++------ .../cluster/raft/ReplicatedLog.java | 15 ++++++++++-- .../cluster/raft/SnapshotManager.java | 2 +- .../cluster/raft/ReplicatedLogImplTest.java | 23 +++++++++++++++---- .../cluster/raft/SnapshotManagerTest.java | 12 ++++------ 5 files changed, 47 insertions(+), 21 deletions(-) diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/AbstractReplicatedLogImpl.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/AbstractReplicatedLogImpl.java index 42af1502ee..64506ee686 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/AbstractReplicatedLogImpl.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/AbstractReplicatedLogImpl.java @@ -256,18 +256,20 @@ public abstract class AbstractReplicatedLogImpl implements ReplicatedLog { } @Override - public void snapshotCommit() { + public void snapshotCommit(final boolean updateDataSize) { snapshottedJournal = null; previousSnapshotIndex = -1; previousSnapshotTerm = -1; - // need to recalc the datasize based on the entries left after precommit. - int newDataSize = 0; - for (ReplicatedLogEntry logEntry : journal) { - newDataSize += logEntry.size(); + if (updateDataSize) { + // need to recalc the datasize based on the entries left after precommit. + int newDataSize = 0; + for (ReplicatedLogEntry logEntry : journal) { + newDataSize += logEntry.size(); + } + LOG.trace("{}: Updated dataSize from {} to {}", logContext, dataSize, newDataSize); + dataSize = newDataSize; } - LOG.trace("{}: Updated dataSize from {} to {}", logContext, dataSize, newDataSize); - dataSize = newDataSize; } @Override diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLog.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLog.java index 1a0b38f778..8cf133c2ab 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLog.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLog.java @@ -186,9 +186,20 @@ public interface ReplicatedLog { void snapshotPreCommit(long snapshotCapturedIndex, long snapshotCapturedTerm); /** - * Sets the Replicated log to state after snapshot success. + * Sets the Replicated log to state after snapshot success. This method is equivalent to + * {@code snapshotCommit(true)}. */ - void snapshotCommit(); + default void snapshotCommit() { + snapshotCommit(true); + } + + /** + * Sets the Replicated log to state after snapshot success. Most users will want to use {@link #snapshotCommit()} + * instead. + * + * @param updateDataSize true if {@link #dataSize()} should also be updated + */ + void snapshotCommit(boolean updateDataSize); /** * Restores the replicated log to a state in the event of a save snapshot failure. diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotManager.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotManager.java index 791a791027..8037fb8d73 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotManager.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotManager.java @@ -265,7 +265,7 @@ public class SnapshotManager implements SnapshotState { //use the term of the temp-min, since we check for isPresent, entry will not be null ReplicatedLogEntry entry = context.getReplicatedLog().get(tempMin); context.getReplicatedLog().snapshotPreCommit(tempMin, entry.getTerm()); - context.getReplicatedLog().snapshotCommit(); + context.getReplicatedLog().snapshotCommit(false); return tempMin; } diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImplTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImplTest.java index 8e9c6c9271..35ecd77d39 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImplTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImplTest.java @@ -57,16 +57,17 @@ public class ReplicatedLogImplTest { MockitoAnnotations.initMocks(this); context = new RaftActorContextImpl(null, null, "test", - new ElectionTermImpl(mockPersistence, "test", LOG), -1, -1, Collections.emptyMap(), + new ElectionTermImpl(mockPersistence, "test", LOG), -1, -1, Collections.emptyMap(), configParams, mockPersistence, applyState -> { }, LOG, MoreExecutors.directExecutor()); } - private void verifyPersist(Object message) throws Exception { + private void verifyPersist(final Object message) throws Exception { verifyPersist(message, new Same(message), true); } @SuppressWarnings({ "unchecked", "rawtypes" }) - private void verifyPersist(Object message, ArgumentMatcher matcher, boolean async) throws Exception { + private void verifyPersist(final Object message, final ArgumentMatcher matcher, final boolean async) + throws Exception { ArgumentCaptor procedure = ArgumentCaptor.forClass(Procedure.class); if (async) { verify(mockPersistence).persistAsync(argThat(matcher), procedure.capture()); @@ -196,7 +197,21 @@ public class ReplicatedLogImplTest { verifyNoMoreInteractions(mockPersistence); } - public ArgumentMatcher match(final DeleteEntries actual) { + @Test + public void testCommitFakeSnapshot() { + ReplicatedLog log = ReplicatedLogImpl.newInstance(context); + + log.append(new SimpleReplicatedLogEntry(0, 1, new MockPayload("0"))); + final int dataSizeAfterFirstPayload = log.dataSize(); + + log.snapshotPreCommit(0,1); + log.snapshotCommit(false); + + assertEquals(0, log.size()); + assertEquals(dataSizeAfterFirstPayload, log.dataSize()); + } + + private static ArgumentMatcher match(final DeleteEntries actual) { return other -> actual.getFromIndex() == other.getFromIndex(); } } diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/SnapshotManagerTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/SnapshotManagerTest.java index 88e9a613ca..4cdec8ca67 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/SnapshotManagerTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/SnapshotManagerTest.java @@ -560,7 +560,7 @@ public class SnapshotManagerTest extends AbstractActorTest { assertEquals("return index", 10L, retIndex); verify(mockReplicatedLog).snapshotPreCommit(10, 5); - verify(mockReplicatedLog).snapshotCommit(); + verify(mockReplicatedLog).snapshotCommit(false); verify(mockRaftActorBehavior, never()).setReplicatedToAllIndex(anyLong()); } @@ -578,7 +578,7 @@ public class SnapshotManagerTest extends AbstractActorTest { assertEquals("return index", -1L, retIndex); verify(mockReplicatedLog, never()).snapshotPreCommit(anyLong(), anyLong()); - verify(mockReplicatedLog, never()).snapshotCommit(); + verify(mockReplicatedLog, never()).snapshotCommit(false); verify(mockRaftActorBehavior, never()).setReplicatedToAllIndex(anyLong()); } @@ -596,7 +596,7 @@ public class SnapshotManagerTest extends AbstractActorTest { assertEquals("return index", -1L, retIndex); verify(mockReplicatedLog, never()).snapshotPreCommit(anyLong(), anyLong()); - verify(mockReplicatedLog, never()).snapshotCommit(); + verify(mockReplicatedLog, never()).snapshotCommit(false); verify(mockRaftActorBehavior, never()).setReplicatedToAllIndex(anyLong()); } @@ -611,7 +611,7 @@ public class SnapshotManagerTest extends AbstractActorTest { assertEquals("return index", -1L, retIndex); verify(mockReplicatedLog, never()).snapshotPreCommit(anyLong(), anyLong()); - verify(mockReplicatedLog, never()).snapshotCommit(); + verify(mockReplicatedLog, never()).snapshotCommit(false); // Trim index is greater than replicatedToAllIndex so should update it. verify(mockRaftActorBehavior).setReplicatedToAllIndex(10L); @@ -635,8 +635,7 @@ public class SnapshotManagerTest extends AbstractActorTest { snapshotManager.trimLog(10); verify(mockReplicatedLog, never()).snapshotPreCommit(anyLong(), anyLong()); - verify(mockReplicatedLog, never()).snapshotCommit(); - + verify(mockReplicatedLog, never()).snapshotCommit(false); } @Test @@ -658,7 +657,6 @@ public class SnapshotManagerTest extends AbstractActorTest { verify(mockReplicatedLog, never()).snapshotPreCommit(10, 5); verify(mockReplicatedLog, never()).snapshotCommit(); - } @Test -- 2.36.6