Fix delete snapshots criteria 03/42303/2
authorTom Pantelis <tpanteli@brocade.com>
Thu, 21 Jul 2016 23:09:47 +0000 (19:09 -0400)
committerTom Pantelis <tpanteli@brocade.com>
Fri, 22 Jul 2016 17:43:42 +0000 (17:43 +0000)
When a snapshot is saved, we attempt to delete prior snapshots as only
the last one is recovered from persistence. For the maxSequenceNr in the
criteria, we use snapshot sequenceNr - snapshot batch count. However
this assumes every snapshot is based on snapshot batch count. We may
snapshot for other reasons, eg install snapshot on follower. This can
leave multiple prior snapshots behind and use up significant disk space.

We should only retain the last snapshot saved so I changed the criteria
to use: maxSequenceNr = the saved snapshot sequenceNr (it's possible the
sequenceNr hasn't changed from th elast snapshot) and maxTimeStamp =
saved snapshot timestamp -1.

Change-Id: I35b1d71ed433d52ecff79ca07a81616e393a7b7f
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotManager.java
opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotState.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupportTest.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorTest.java
opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/SnapshotManagerTest.java

index b35f4ba..5173dc8 100644 (file)
@@ -60,7 +60,7 @@ class RaftActorSnapshotMessageSupport {
         } else if (message instanceof CaptureSnapshotReply) {
             onCaptureSnapshotReply(((CaptureSnapshotReply) message).getSnapshot());
         } else if (COMMIT_SNAPSHOT.equals(message)) {
-            context.getSnapshotManager().commit(-1);
+            context.getSnapshotManager().commit(-1, -1);
         } else if (message instanceof GetSnapshot) {
             onGetSnapshot(sender);
         } else {
@@ -84,11 +84,11 @@ class RaftActorSnapshotMessageSupport {
     }
 
     private void onSaveSnapshotSuccess(SaveSnapshotSuccess success) {
-        log.info("{}: SaveSnapshotSuccess received for snapshot", context.getId());
-
         long sequenceNumber = success.metadata().sequenceNr();
 
-        context.getSnapshotManager().commit(sequenceNumber);
+        log.info("{}: SaveSnapshotSuccess received for snapshot, sequenceNr: {}", context.getId(), sequenceNumber);
+
+        context.getSnapshotManager().commit(sequenceNumber, success.metadata().timestamp());
     }
 
     private void onApplySnapshot(ApplySnapshot message) {
index dd52c38..a362c6f 100644 (file)
@@ -77,8 +77,8 @@ public class SnapshotManager implements SnapshotState {
     }
 
     @Override
-    public void commit(final long sequenceNumber) {
-        currentState.commit(sequenceNumber);
+    public void commit(final long sequenceNumber, long timeStamp) {
+        currentState.commit(sequenceNumber, timeStamp);
     }
 
     @Override
@@ -177,7 +177,7 @@ public class SnapshotManager implements SnapshotState {
         }
 
         @Override
-        public void commit(final long sequenceNumber) {
+        public void commit(final long sequenceNumber, long timeStamp) {
             LOG.debug("commit should not be called in state {}", this);
         }
 
@@ -385,7 +385,7 @@ public class SnapshotManager implements SnapshotState {
     private class Persisting extends AbstractSnapshotState {
 
         @Override
-        public void commit(final long sequenceNumber) {
+        public void commit(final long sequenceNumber, long timeStamp) {
             LOG.debug("{}: Snapshot success -  sequence number: {}", persistenceId(), sequenceNumber);
 
             if(applySnapshot != null) {
@@ -414,8 +414,8 @@ public class SnapshotManager implements SnapshotState {
                 context.getReplicatedLog().snapshotCommit();
             }
 
-            context.getPersistenceProvider().deleteSnapshots(new SnapshotSelectionCriteria(
-                    sequenceNumber - context.getConfigParams().getSnapshotBatchCount(), Long.MAX_VALUE, 0L, 0L));
+            context.getPersistenceProvider().deleteSnapshots(new SnapshotSelectionCriteria(sequenceNumber,
+                    timeStamp - 1, 0L, 0L));
 
             context.getPersistenceProvider().deleteMessages(lastSequenceNumber);
 
index 5d1304f..a6bcddf 100644 (file)
@@ -57,8 +57,9 @@ public interface SnapshotState {
      * Commit the snapshot by trimming the log
      *
      * @param sequenceNumber
+     * @param timeStamp
      */
-    void commit(long sequenceNumber);
+    void commit(long sequenceNumber, long timeStamp);
 
     /**
      * Rollback the snapshot
index 32b2e46..406ed3c 100644 (file)
@@ -116,9 +116,10 @@ public class RaftActorSnapshotMessageSupportTest {
     public void testOnSaveSnapshotSuccess() {
 
         long sequenceNumber = 100;
-        sendMessageToSupport(new SaveSnapshotSuccess(new SnapshotMetadata("foo", sequenceNumber, 1234L)));
+        long timeStamp = 1234L;
+        sendMessageToSupport(new SaveSnapshotSuccess(new SnapshotMetadata("foo", sequenceNumber, timeStamp)));
 
-        verify(mockSnapshotManager).commit(eq(sequenceNumber));
+        verify(mockSnapshotManager).commit(eq(sequenceNumber), eq(timeStamp));
     }
 
     @Test
@@ -135,7 +136,7 @@ public class RaftActorSnapshotMessageSupportTest {
 
         sendMessageToSupport(RaftActorSnapshotMessageSupport.COMMIT_SNAPSHOT);
 
-        verify(mockSnapshotManager).commit(eq(-1L));
+        verify(mockSnapshotManager).commit(eq(-1L), eq(-1L));
     }
 
     @Test
index 6565c59..24ebad5 100644 (file)
@@ -659,7 +659,7 @@ public class RaftActorTest extends AbstractActorTest {
                 assertTrue(leaderActor.getRaftActorContext().getSnapshotManager().isCapturing());
 
                 // The commit is needed to complete the snapshot creation process
-                leaderActor.getRaftActorContext().getSnapshotManager().commit(-1);
+                leaderActor.getRaftActorContext().getSnapshotManager().commit(-1, -1);
 
                 // capture snapshot reply should remove the snapshotted entries only
                 assertEquals(3, leaderActor.getReplicatedLog().size());
@@ -762,7 +762,7 @@ public class RaftActorTest extends AbstractActorTest {
                 assertTrue(followerActor.getRaftActorContext().getSnapshotManager().isCapturing());
 
                 // The commit is needed to complete the snapshot creation process
-                followerActor.getRaftActorContext().getSnapshotManager().commit(-1);
+                followerActor.getRaftActorContext().getSnapshotManager().commit(-1, -1);
 
                 // capture snapshot reply should remove the snapshotted entries only till replicatedToAllIndex
                 assertEquals(3, followerActor.getReplicatedLog().size()); //indexes 5,6,7 left in the log
index e35c9bd..c619917 100644 (file)
@@ -413,7 +413,7 @@ public class SnapshotManagerTest extends AbstractActorTest {
 
         assertEquals(true, snapshotManager.isCapturing());
 
-        snapshotManager.commit(100L);
+        snapshotManager.commit(100L, 1234L);
 
         assertEquals(false, snapshotManager.isCapturing());
 
@@ -425,9 +425,8 @@ public class SnapshotManagerTest extends AbstractActorTest {
 
         verify(mockDataPersistenceProvider).deleteSnapshots(criteriaCaptor.capture());
 
-        assertEquals(90, criteriaCaptor.getValue().maxSequenceNr()); // sequenceNumber = 100
-                                                                     // config snapShotBatchCount = 10
-                                                                     // therefore maxSequenceNumber = 90
+        assertEquals(100L, criteriaCaptor.getValue().maxSequenceNr());
+        assertEquals(1233L, criteriaCaptor.getValue().maxTimestamp());
 
         MessageCollectorActor.expectFirstMatching(actorRef, SnapshotComplete.class);
     }
@@ -438,7 +437,7 @@ public class SnapshotManagerTest extends AbstractActorTest {
         snapshotManager.captureToInstall(new MockRaftActorContext.MockReplicatedLogEntry(6, 9,
                 new MockRaftActorContext.MockPayload()), -1, "follower-1");
 
-        snapshotManager.commit(100L);
+        snapshotManager.commit(100L, 0);
 
         verify(mockReplicatedLog, never()).snapshotCommit();
 
@@ -450,7 +449,7 @@ public class SnapshotManagerTest extends AbstractActorTest {
 
     @Test
     public void testCommitBeforeCapture(){
-        snapshotManager.commit(100L);
+        snapshotManager.commit(100L, 0);
 
         verify(mockReplicatedLog, never()).snapshotCommit();
 
@@ -470,9 +469,9 @@ public class SnapshotManagerTest extends AbstractActorTest {
 
         snapshotManager.persist(new byte[]{}, Runtime.getRuntime().totalMemory());
 
-        snapshotManager.commit(100L);
+        snapshotManager.commit(100L, 0);
 
-        snapshotManager.commit(100L);
+        snapshotManager.commit(100L, 0);
 
         verify(mockReplicatedLog, times(1)).snapshotCommit();
 

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