Do not reset datasize after a fake snapshot 79/91679/11
authortadei.bilan <tadei.bilan@pantheon.tech>
Mon, 27 Jul 2020 10:18:29 +0000 (13:18 +0300)
committerRobert Varga <robert.varga@pantheon.tech>
Thu, 22 Oct 2020 18:45:40 +0000 (20:45 +0200)
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 <tadei.bilan@pantheon.tech>
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/AbstractReplicatedLogImpl.java
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLog.java
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotManager.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImplTest.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/SnapshotManagerTest.java

index 42af150..64506ee 100644 (file)
@@ -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
index 1a0b38f..8cf133c 100644 (file)
@@ -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.
index 791a791..8037fb8 100644 (file)
@@ -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;
             }
 
index 8e9c6c9..35ecd77 100644 (file)
@@ -57,16 +57,17 @@ public class ReplicatedLogImplTest {
         MockitoAnnotations.initMocks(this);
 
         context = new RaftActorContextImpl(null, null, "test",
-                new ElectionTermImpl(mockPersistence, "test", LOG), -1, -1, Collections.<String,String>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> 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<DeleteEntries> 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<DeleteEntries> match(final DeleteEntries actual) {
         return other -> actual.getFromIndex() == other.getFromIndex();
     }
 }
index 88e9a61..4cdec8c 100644 (file)
@@ -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

©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.