akka-persistence: LocalSnapshotStore ignores timestamps in SnapshotSelectionCriteria

[Stephen McDonald]

Hi Hakkers,

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:

https://github.com/akka/akka/commit/63baaf1b2b3d0fc6c1971307fd1e66d74106c968

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?

Thanks for Akka, it's amazing!

--

Stephen McDonald
http://jupo.org

[Stephen McDonald]

BTW for anyone who hits this issue and needs an immediate solution, here's how I fixed it within my own project. Couldn't quite subclass as I mentioned earlier, due to LocalSnapshotStore being private, so I made a copy and tweaked as required to support timestamp handling in snapshot criterias:

https://github.com/stephenmcd/curiodb/commit/d034599d1f38077106c87a69b543a479c24efcb1

[Patrik Nordwall]

Why do you need to specify the timestamp when you know the sequence number?

Anyway, has this been reported as an github issue?

/Patrik

--
>>>>>>>>>> Read the docs: http://akka.io/docs/
>>>>>>>>>> Check the FAQ: http://doc.akka.io/docs/akka/current/additional/faq.html
>>>>>>>>>> Search the archives: https://groups.google.com/group/akka-user
---
You received this message because you are subscribed to the Google Groups "Akka User List" group.
To unsubscribe from this group and stop receiving emails from it, send an email to akka-user+...@googlegroups.com.
To post to this group, send email to akka...@googlegroups.com.
Visit this group at http://groups.google.com/group/akka-user.
For more options, visit https://groups.google.com/d/optout.

--

Patrik Nordwall
Typesafe -  Reactive apps on the JVM
Twitter: @patriknw

[Konrad Malawski]

Hi there,

sorry I missed this thread previously.

This change has some background story to it, but perhaps we missed something when Criteria is used for the delete...

I'll open a bug and have a look at it. The reason was to abandon the deleteSnapshot(seqNr, timestamp) API, as the timestamp is actually not needed in this API.

The gitter discussion around this was on Jun 16th if you're curious for reasons around this change.

-- 

Cheers,

Konrad 'ktoso’ Malawski
Akka @ Typesafe

[Konrad Malawski]

By looking at the patch it seems it's a bug indeed - would you submit the patch as a PR Stephen?

I opened up an issue, so you could open a PR referencing the issue nr: https://github.com/akka/akka/issues/18112

Thanks a lot for finding this and the patch!

-- 

Cheers,

Konrad 'ktoso’ Malawski
Akka @ Typesafe

On 3 August 2015 at 13:39:13, Patrik Nordwall (patrik....@gmail.com) wrote:

[Patrik Nordwall]

On Mon, Aug 3, 2015 at 1:55 PM, Konrad Malawski <konrad....@typesafe.com> wrote:

Hi there,

sorry I missed this thread previously.

This change has some background story to it, but perhaps we missed something when Criteria is used for the delete...

I'll open a bug and have a look at it. The reason was to abandon the deleteSnapshot(seqNr, timestamp) API, as the timestamp is actually not needed in this API.

I think this issue was related to deleteSnapshots, not deleteSnapshot

[Patrik Nordwall]

On Mon, Aug 3, 2015 at 1:59 PM, Konrad Malawski <konrad....@typesafe.com> wrote:

By looking at the patch it seems it's a bug indeed - would you submit the patch as a PR Stephen?

I opened up an issue, so you could open a PR referencing the issue nr: https://github.com/akka/akka/issues/18112

Thanks!

[Stephen McDonald]

On Mon, Aug 3, 2015 at 9:39 PM, Patrik Nordwall <patrik....@gmail.com> wrote:

Why do you need to specify the timestamp when you know the sequence number?

While debugging this I also discovered the sequence number was always the same - I don't have enough understanding to know whether this was a bug or a mistake with my usage (I actually assumed the latter and then focused on the timestamp issue).

 

You received this message because you are subscribed to a topic in the Google Groups "Akka User List" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/akka-user/0CpgVgi0PE8/unsubscribe.
To unsubscribe from this group and all its topics, send an email to akka-user+...@googlegroups.com.

To post to this group, send email to akka...@googlegroups.com.
Visit this group at http://groups.google.com/group/akka-user.
For more options, visit https://groups.google.com/d/optout.

--

Stephen McDonald
http://jupo.org