Deleting local snapshots using criteria should take into account the timestamp

[ktoso]

Discovered by Stephen McDonald in the akka-user thread: https://groups.google.com/forum/#!searchin/akka-user/localsnapshotstore$20ignores$20timestamps/akka-user/0CpgVgi0PE8/6gYW5v6zjToJ

I came across some surprising behaviour with the LocalSnapshotStore - I was trying to implement what I imagine would be a common pattern: remove older snapshots each time a new snapshot is created. I did this with deleteSnapshots(SnapshotSelectionCriteria(meta.sequenceNr, meta.timestamp - 1)), only to find it would delete all snapshots for the given persistence ID and sequenceNr, regardless of timestamp given.

After staring at my own code for a very long time, I looked at the source for LocalSnapshotStore and found that it seems to ignore timestamps altogether:

https://github.com/akka/akka/blob/master/akka-persistence/src/main/scala/akka/persistence/snapshot/local/LocalSnapshotStore.scala#L161

I initially thought this to be a bug, but looking at the history it appears to have been intentionally removed:

63baaf1

I was able to fix this with a minor change in the attached diff. Is there any interest in this fix? Or am I naive to some history around this?

I guess if it's the intended behaviour, I can easily enough subclass it and override the relevant bits to take timestamps into account - does that sound reasonable?

Attached diff:

diff --git a/akka-persistence/src/main/scala/akka/persistence/snapshot/local/LocalSnapshotStore.scala b/akka-persistence/src/main/scala/akka/persistence/snapshot/local/LocalSnapshotStore.scala
index 4dc1ff6..b513816 100644
--- a/akka-persistence/src/main/scala/akka/persistence/snapshot/local/LocalSnapshotStore.scala
+++ b/akka-persistence/src/main/scala/akka/persistence/snapshot/local/LocalSnapshotStore.scala
@@ -25,6 +25,7 @@ import scala.util._
  * Local filesystem backed snapshot store.
  */
 private[persistence] class LocalSnapshotStore extends SnapshotStore with ActorLogging {
+
   private val FilenamePattern = """^snapshot-(.+)-(\d+)-(\d+)""".r

   import akka.util.Helpers._
@@ -82,7 +83,7 @@ private[persistence] class LocalSnapshotStore extends SnapshotStore with ActorLo

   private def snapshotFiles(metadata: SnapshotMetadata): immutable.Seq[File] = {
     // pick all files for this persistenceId and sequenceNr, old journals could have created multiple entries with appended timestamps
-    snapshotDir.listFiles(new SnapshotSeqNrFilenameFilter(metadata.persistenceId, metadata.sequenceNr)).toVector
+    snapshotDir.listFiles(new SnapshotSeqNrFilenameFilter(metadata.persistenceId, metadata.sequenceNr, metadata.timestamp)).toVector
   }

   @scala.annotation.tailrec
@@ -156,13 +157,14 @@ private[persistence] class LocalSnapshotStore extends SnapshotStore with ActorLo
     }
   }

-  private final class SnapshotSeqNrFilenameFilter(persistenceId: String, sequenceNr: Long) extends FilenameFilter {
-    private final def matches(pid: String, snr: String): Boolean =
-      pid.equals(URLEncoder.encode(persistenceId)) && Try(snr.toLong == sequenceNr).getOrElse(false)
+  private final class SnapshotSeqNrFilenameFilter(persistenceId: String, sequenceNr: Long, timestamp: Long) extends FilenameFilter {
+    private final def matches(pid: String, snr: String, tms: String): Boolean = {
+      pid.equals(URLEncoder.encode(persistenceId)) && Try(snr.toLong == sequenceNr).getOrElse(false) && Try(tms.toLong <= timestamp).getOrElse(false)
+    }

     def accept(dir: File, name: String): Boolean =
       name match {
-        case FilenamePattern(pid, snr, tms) ⇒ matches(pid, snr)
+        case FilenamePattern(pid, snr, tms) ⇒ matches(pid, snr, tms)
         case _                              ⇒ false
       }

[ktoso]

// cc @stephenmcd

[stephenmcd]

Thanks Konrad. Any comment on the patch? Would you like me to set up a PR?

[stephenmcd]

Ignore my previous message here, I read this before the mailing list thread.

I'll set up a PR.

[ktoso]

Thanks! :-)

[patriknw]

I think this issue is invalid. The SnapshotSeqNrFilenameFilter that was pointed out to ignore the timestamp is only used when deleting one single snapshot by its sequence number. User api does not include a timestamp.

I can't see anything wrong with the deleteSnapshots with a criteria. That one takes both seqNr and timestamp into account.

[stephenmcd]

Hi @patriknw, thanks for looking at this.

I can't see anything wrong with the deleteSnapshots with a criteria. That one takes both seqNr and timestamp into account.

On the surface it looks that way, but I think there's still a problem - deleteSnapshots correctly builds a list of files matching the given sequence number and timestamp, but it then calls deleteAsync for each of these, which only takes into account the sequence number, just as you described. So in the case of multiple snapshots existing with the same sequence number, but different timestamps, they'll all be deleted - so yes, while the timestamp is initially taken into account, it ends up being ignored eventually.

Also as far as I've seen with LocalSnapshotStore, sequence numbers are never incremented, so this problem is bound to occur when trying to delete all snapshots other than the latest for a given persistence ID.

Given that, do you think my patch is the correct fix, or something else? For my project I've copied LocalSnapshotStore locally and used the above patch to avoid any data loss, and it's working correctly.

Thanks again, and sorry if I've overlooked something.

[patriknw]

Thanks for clarifying. Now it makes sense.

[stephenmcd]

No problem at all.

[stephenmcd]

Just confirming the fix in 0cd94e2 resolves the issue for me - thanks a lot @patriknw!

[patriknw]

thanks for reporting and confirming