From 59918fc863252de0e51e5167f0cb57f697b76c13 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Thu, 21 Jul 2016 19:09:47 -0400 Subject: [PATCH] Fix delete snapshots criteria 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 --- .../raft/RaftActorSnapshotMessageSupport.java | 8 ++++---- .../controller/cluster/raft/SnapshotManager.java | 12 ++++++------ .../controller/cluster/raft/SnapshotState.java | 3 ++- .../raft/RaftActorSnapshotMessageSupportTest.java | 7 ++++--- .../controller/cluster/raft/RaftActorTest.java | 4 ++-- .../cluster/raft/SnapshotManagerTest.java | 15 +++++++-------- 6 files changed, 25 insertions(+), 24 deletions(-) diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java index b35f4babb1..5173dc89f7 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupport.java @@ -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) { 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 dd52c38685..a362c6f683 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 @@ -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); diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotState.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotState.java index 5d1304fe75..a6bcddfc13 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotState.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/SnapshotState.java @@ -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 diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupportTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupportTest.java index 32b2e465f6..406ed3c315 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupportTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorSnapshotMessageSupportTest.java @@ -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 diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorTest.java index 6565c59b5f..24ebad57ad 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/RaftActorTest.java @@ -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 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 e35c9bd44a..c619917fd4 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 @@ -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(); -- 2.36.6