feat: delete events for snapshotWhen/shouldSnapshot predicate #32231

sebastian-alfers · 2023-11-13T17:04:02Z

javadsl/EventSourcedBehavior.shouldSnapshot and scaladsl/EventSourcedBehavior.snapshotWhen both got deprecated and should be used via .withRetention(RetentionCriteria.shouldSnapshot(predicate).

Disablers the combined usage of retention and predicate based api. This means (like before) snapshot deletion can not be triggered when using the predicate base api.

Fixes #29685.

patriknw

looking very good

akka-persistence-typed/src/main/scala/akka/persistence/typed/javadsl/EventSourcedBehavior.scala

sebastian-alfers · 2023-11-14T09:52:33Z

Since it seems that the direction of this PR is about right, I am going to add docs, mima ect now.

sebastian-alfers · 2023-11-20T10:33:25Z

@patriknw I think this is ready to be reviewed.

patriknw

looking good

...tests/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedBehaviorRetentionSpec.scala

akka-docs/src/main/paradox/typed/persistence-snapshot.md

...tests/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedBehaviorRetentionSpec.scala

johanandren · 2023-11-29T09:37:01Z

akka-persistence-typed/src/main/scala/akka/persistence/typed/javadsl/EventSourcedBehavior.scala

+   *
+   * @return `true` if events should be deleted after `shouldSnapshot` evaluates to true
+   */
+  def deleteEventsOnSnapshot: Boolean = false


For the Java API this is a bit weirder because it doesn't have context except for the API-docs. I think a user could easily end up missing the shouldSnapshot and just override just this expecting that it will delete events on snapshot.

public RetentionCriteria retentionCriteria() { return RetentionCriteria.snapshotEvery(100, 2); } public bool deleteEventsOnSnapshot() { return true; }

I think that we need to either make either delete flag being true work or try to make this name more clear about being only for predicate based snapshot

Initially the name of this method was deleteEventsOnShouldSnapshot which got changed into the current name.

I think because of the current implementation we should separate the two flags and their respective impact.

The main problem here is the api of def shouldSnapshot(@unused state: State, @unused event: Event, @unused sequenceNr: Long): Boolean which should be evolved to a proper type, similar to withRetention. Maybe this is the way to go by adding @deprecated to the api and introducing a new method+type for this? This should then also be made for the scala api.

Hmm, tricky to get it right without overdoing it. A type to return also means users has to figure out how to construct that, but maybe that is worth it because it makes what the flag refers to obvious.

Maybe naming it back to deleteEventsOnShouldSnapshot @patriknw? ;-)

Taking a step back, it's kind of weird to have the snapshot predicate and snapshot via RetentionCriteria as two separate things. We could add the snapshotWhen as another RetentionCriteria, and use the same approach for withDeleteEventsOnSnapshot.

Maybe that would loose the possibility to use both snapshotEvery(numberOfEvents) and snapshotWhen(predicate), but that is probably an unusual combination anyway.

A user could always do the every N as a part of their predicate, by looking at seqnr, if they want both.

I like the idea of evolving the predicate api. But is a snapshotWhen predicate actually something that belongs to a RetentionCriteria? To me retention refers to the fact if a snapshot should be stored, and if so how many of them. I think we need a more composable api:

define when a snapshot is triggered (predicate and/or numberOfEvents)

if triggered, what is the retention of snapshots?

delete events after a snapshot is triggered

To me this are three different properties that may be composed as needed (with some validation). But this probably goes into "not overdoing it".

For now I will change this PR and put snapshotWhen into RetentionCriteria to move it forward, we can see?

I think it's fine that we add some more for when to snapshot in RetentionCriteria. That is anyway the normal way to define that snapshots should be enabled.

Originally I think retention referred to deletion of events, but we didn't allow deletion unless there are snapshots. Yeah, maybe not perfect name, but that is what it is.

For now I will change this PR and put snapshotWhen into RetentionCriteria to move it forward

Agreed

Do you agree that the new snapshotWhen should also receive a keepNSnapshots param, like def snapshotEvery(numberOfEvents: Int, keepNSnapshots: Int): SnapshotCountRetentionCriteria?

If not, after this pr, it will not be possible anymore to delete snapshots when using the (new) predicate based api because we remove option to combine snapshotEvery and predicate ways to define snapshot triggering.

Or we rely on only-one-snapshot to clean up snapshots?

johanandren

Added some notes but I guess this is in active dev so maybe you let us know when you want it reviewed again

akka-persistence-typed/src/main/scala/akka/persistence/typed/scaladsl/RetentionCriteria.scala

...ce-typed/src/test/scala/docs/akka/persistence/typed/BasicPersistentBehaviorCompileOnly.scala

...tests/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedBehaviorRetentionSpec.scala

akka-persistence-typed/src/main/scala/akka/persistence/typed/internal/Running.scala

patriknw

looking good

...ce-typed/src/test/scala/docs/akka/persistence/typed/BasicPersistentBehaviorCompileOnly.scala

akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala

sebastian-alfers · 2023-12-09T12:59:30Z

...-testkit/src/test/scala/akka/persistence/testkit/scaladsl/UnpersistentEventSourcedSpec.scala

-            case _                    => false
-          }
-          .withRetention(RetentionCriteria.snapshotEvery(numberOfEvents = 3, keepNSnapshots = 2))
+          .withRetention(RetentionCriteria.snapshotEvery(numberOfEvents = 2, keepNSnapshots = 2))


This had to change because now we do not allow anymore to combine snapshotWhen and snapshotEvery

For it to be equivalent with the existing it would be something like snapshotWhen((_, event, currentSeqNr) => event == SnapshotMade || currentSeqNr % 3 == 0)

Interesting, this means we will loose the keepNSnapshots = 2 of the current api usage.

Not sure it is important to keep it snapshotting at the exact same points here though, so maybe fine.

akka-docs/src/main/paradox/typed/persistence-snapshot.md

johanandren · 2023-12-11T15:55:11Z

...-testkit/src/test/scala/akka/persistence/testkit/scaladsl/UnpersistentEventSourcedSpec.scala

-            case _                    => false
-          }
-          .withRetention(RetentionCriteria.snapshotEvery(numberOfEvents = 3, keepNSnapshots = 2))
+          .withRetention(RetentionCriteria.snapshotEvery(numberOfEvents = 2, keepNSnapshots = 2))


For it to be equivalent with the existing it would be something like snapshotWhen((_, event, currentSeqNr) => event == SnapshotMade || currentSeqNr % 3 == 0)

johanandren · 2023-12-11T15:55:49Z

akka-persistence-typed-tests/src/test/scala/akka/persistence/typed/crdt/CounterSpec.scala

@@ -51,9 +54,9 @@ object CounterSpec {
          (counter, event) => {
            eventProbe.foreach(_ ! event)
            counter.applyOperation(event)
-          }).snapshotWhen { (_, _, seqNr) =>
+          }).withRetention(RetentionCriteria.snapshotWhen[Any, Any] { (_, _, seqNr) =>


The explicit type params are needed now, even with the explicit types on EventSourcedBehavior.apply above?

Yes. When I remove [Any, Any] I do get:

akka-persistence-typed-tests/src/test/scala/akka/persistence/typed/crdt/CounterSpec.scala:57:65: missing parameter type [error] }).withRetention(RetentionCriteria.snapshotWhen { (_, _, seqNr) =>

johanandren · 2023-12-11T15:57:38Z

...tests/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedBehaviorRetentionSpec.scala

+            pid,
+            snapshotSignalProbe = Some(snapshotSignalProbe.ref),
+            eventSignalProbe = Some(eventProbe.ref))
+            .withRetention(RetentionCriteria.snapshotWhen[State, Event]((_, _, _) => true).withDeleteEventsOnSnapshot)


Here we have explicit types on the returned ESB, the compiler doesn't infer those?

Same error as above:

akka-persistence-typed-tests/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedBehaviorRetentionSpec.scala:141:60: missing parameter type [error] .withRetention(RetentionCriteria.snapshotWhen((_, _, _) => true).withDeleteEventsOnSnapshot)

hmm, this is unfortunate. There are no types on the RetentionCriteria base trait, ofc. Therefore the types of the snapshotWhen are unrelated to the types of the EventSourcedBehavior. For example this would be wrong, but compiles fine:

.withRetention(RetentionCriteria.snapshotWhen[String, Int]((_, _, _) => true).withDeleteEventsOnSnapshot)

johanandren · 2023-12-11T16:00:56Z

akka-persistence-typed/src/main/scala/akka/persistence/typed/javadsl/RetentionCriteria.scala

+   *
+   * Can not be combined with the `snapshotEvery` api.
+   */
+  def snapshotWhen[State, Event](predicate: (State, Event, Long) => Boolean): SnapshotPredicateCriteria =


Ah, now I see why the types cannot be inferred, they aren't tied to anything except the lambda. Hmmm, that's annoying but I don't see an obvious solution, other than introducing type params to RetentionCriteria or pulling the factory methods up to EventSourcedBehavior and neither of those seem like a great idea.

@patriknw any other ideas how we could get this to align, note that it is not only cumbersome but also somewhat unsafe because with the current PR API you can create a SnapshotPredicate for other event and state types than your actual ESB uses.

I agree, I think this is a stopper for placing the predicate function in the RetentionCriteria. We can't really add type parameters to RetentionCritiera in a source compatible way.

Sorry, but I think we are back to the old approach. If we see that the SnapshotPredicateCriteria simplifies the internal implementation we could keep that as an internal implementation, but maybe we should just revert back to where we were before we suggested the RetentionCriteria?

I reverted this PR to the old approach.

johanandren · 2023-12-11T16:01:31Z

...ersistence-typed/src/test/java/jdocs/akka/persistence/typed/BasicPersistentBehaviorTest.java

+    @Override // override retentionCriteria in EventSourcedBehavior
+    public RetentionCriteria retentionCriteria() {
+      return RetentionCriteria
+              .snapshotWhen((state, event, currentSequenceNr) -> event instanceof BookingCompleted)


So here the types end up being (Object, Object, Long)?

All three seem Object as this is what intellij infers me as the type. I assume that its only scala that is more picky in having the requirement that all types resolvable in an anonymous lambda.

Co-authored-by: Johan Andrén <johan@markatta.com>

sebastian-alfers · 2024-02-05T19:11:34Z

Resolved the merge conflicts with main, so ready to be re-reviewed I guess.

johanandren

LGTM, thanks for exploring the alternative API idea even if it turned out to be a dead end!

patriknw reviewed Nov 14, 2023

View reviewed changes

akka-persistence-typed/src/main/scala/akka/persistence/typed/javadsl/EventSourcedBehavior.scala Outdated Show resolved Hide resolved

patriknw reviewed Nov 20, 2023

View reviewed changes

...tests/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedBehaviorRetentionSpec.scala Show resolved Hide resolved

akka-docs/src/main/paradox/typed/persistence-snapshot.md Outdated Show resolved Hide resolved

sebastian-alfers commented Nov 20, 2023

View reviewed changes

...tests/src/test/scala/akka/persistence/typed/scaladsl/EventSourcedBehaviorRetentionSpec.scala Outdated Show resolved Hide resolved

johanandren reviewed Nov 29, 2023

View reviewed changes

johanandren reviewed Dec 5, 2023

View reviewed changes

sebastian-alfers marked this pull request as draft December 5, 2023 16:56

patriknw reviewed Dec 8, 2023

View reviewed changes

...ce-typed/src/test/scala/docs/akka/persistence/typed/BasicPersistentBehaviorCompileOnly.scala Outdated Show resolved Hide resolved

sebastian-alfers commented Dec 9, 2023

View reviewed changes

akka-persistence-testkit/src/main/scala/akka/persistence/testkit/internal/Unpersistent.scala Outdated Show resolved Hide resolved

sebastian-alfers commented Dec 9, 2023

View reviewed changes

sebastian-alfers marked this pull request as ready for review December 10, 2023 13:27

johanandren reviewed Dec 11, 2023

View reviewed changes

sebastian-alfers force-pushed the delete-events-spapshot-predicate branch from 8903224 to 8ff009f Compare December 19, 2023 11:32

feat: delete events for snapshotWhen/shouldSnapshot predicate

f56e72b

Co-authored-by: Johan Andrén <johan@markatta.com>

sebastian-alfers force-pushed the delete-events-spapshot-predicate branch from 2c315bc to f56e72b Compare February 5, 2024 17:11

make mima happy, fix import

4e75305

johanandren approved these changes Feb 20, 2024

View reviewed changes

johanandren merged commit e49b39e into akka:main Feb 20, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: delete events for snapshotWhen/shouldSnapshot predicate #32231

feat: delete events for snapshotWhen/shouldSnapshot predicate #32231

sebastian-alfers commented Nov 13, 2023 •

edited

patriknw left a comment

sebastian-alfers commented Nov 14, 2023 •

edited

sebastian-alfers commented Nov 20, 2023

patriknw left a comment

johanandren Nov 29, 2023

sebastian-alfers Nov 29, 2023

johanandren Nov 29, 2023

sebastian-alfers Nov 29, 2023

patriknw Nov 29, 2023

johanandren Nov 29, 2023

sebastian-alfers Nov 29, 2023

patriknw Nov 29, 2023

sebastian-alfers Dec 7, 2023

sebastian-alfers Dec 8, 2023

johanandren left a comment

patriknw left a comment

sebastian-alfers Dec 9, 2023

johanandren Dec 11, 2023

sebastian-alfers Dec 11, 2023

johanandren Dec 19, 2023

johanandren Dec 11, 2023

johanandren Dec 11, 2023

sebastian-alfers Dec 11, 2023

johanandren Dec 11, 2023

sebastian-alfers Dec 11, 2023

patriknw Dec 19, 2023

johanandren Dec 11, 2023

johanandren Dec 12, 2023

patriknw Dec 19, 2023

sebastian-alfers Jan 16, 2024

johanandren Dec 11, 2023

sebastian-alfers Dec 11, 2023 •

edited

sebastian-alfers commented Feb 5, 2024

johanandren left a comment

feat: delete events for snapshotWhen/shouldSnapshot predicate #32231

feat: delete events for snapshotWhen/shouldSnapshot predicate #32231

Conversation

sebastian-alfers commented Nov 13, 2023 • edited

patriknw left a comment

Choose a reason for hiding this comment

sebastian-alfers commented Nov 14, 2023 • edited

sebastian-alfers commented Nov 20, 2023

patriknw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanandren left a comment

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastian-alfers Dec 11, 2023 • edited

Choose a reason for hiding this comment

sebastian-alfers commented Feb 5, 2024

johanandren left a comment

Choose a reason for hiding this comment

sebastian-alfers commented Nov 13, 2023 •

edited

sebastian-alfers commented Nov 14, 2023 •

edited

sebastian-alfers Dec 11, 2023 •

edited