Allow persisting events when recovery has completed #3366

ismaelhamed · 2018-03-17T12:49:29Z

DO NOT MERGE before #3372

This PR adds support for persisting when recovery is completed, before any commands are received. It's still not possible to persist new events when replaying events. Replaying events should typically be free of external side effects so it doesn't make much sense to support persisting during the actual replay, but there might be some use cases for doing it right afterwards.

Original issues: #21736 and #22609
Original PR: #21893

~~core implementation~~
~~unit tests~~

ismaelhamed · 2018-03-17T12:50:02Z

src/core/Akka.Persistence/Eventsourced.Recovery.cs

@@ -17,17 +17,15 @@ namespace Akka.Persistence

    internal class EventsourcedState
    {
-        public EventsourcedState(string name, bool isRecoveryRunning, StateReceive stateReceive)
+        public EventsourcedState(string name, Func<bool> isRecoveryRunning, StateReceive stateReceive)


Is this the best option?

Question: how often is the EventsourcedState object allocated? From a functional standpoint it should be fine, but if we allocate a new EventsourcedState instance regularly then anonymous method allocations could add up. I'm not familiar enough with Akka.Persistence's internals to know off-hand. Do you or @Horusiath have some idea?

@Aaronontheweb not sure about this one, honestly. Maybe @Horusiath wants to weigh in.

ismaelhamed · 2018-03-17T12:51:01Z

src/core/Akka.Persistence/Eventsourced.Recovery.cs


-            return new EventsourcedState("replay started", true, (receive, message) =>
+            return new EventsourcedState("replay started", () => recoveryRunning, (receive, message) =>


Implicitly capture closure

Aaronontheweb · 2018-04-14T20:51:48Z

@ismaelhamed still waiting on more changes first?

ismaelhamed · 2018-04-15T08:36:50Z

@Aaronontheweb code is complete, but I'd appreciate comments on those remarks. Until then, this is still WIP

Aaronontheweb · 2018-04-17T14:46:55Z

Thanks, I'll take a look

ismaelhamed · 2018-04-17T15:29:35Z

src/core/Akka.Persistence/Eventsourced.Recovery.cs

-
-        public bool IsRecoveryRunning { get; }
-
+        public Func<bool> IsRecoveryRunning { get; }


This breaks the API?!

Nah, this is an internal API. None of the third party Akka.Persistence plugins have access to this so we're able to preserve binary compatibility here. It's only the Akka.Persistence engine itself that consumes this directly so I think you're fine.

Aaronontheweb

Have some questions about the specs and performance impacts that I'd like to have answered before pushing this through into a patch.

Aaronontheweb · 2018-04-17T20:25:06Z

src/core/Akka.Persistence.Tests/PersistentActorSpec.cs

+            ExpectMsgInOrder("a-1", "a-2", "rc-1", "rc-2", "rc-3");
+            persistentActor.Tell(new Cmd("invalid"));
+            persistentActor.Tell(GetState.Instance);
+            ExpectMsgInOrder("a-1", "a-2", "rc-1", "rc-2", "rc-3", "invalid");


@ismaelhamed shouldn't there be an invalid-recovery event recovered here?

@Aaronontheweb No, the point of this PR is twofold: First, prevent commands from interrupting the events being persisted in RecoveryCompleted. Second, make sure an actor blows up if Persist() is invoked when replaying events (due to "invalid-recovery" event in this case).

So basically, persist is allowed in RecoveryCompleted, but it will fail-fast during recovery. You'll get a nice exception along with the intrusive ("invalid") message in OnRecoveryFailure though.

Aaronontheweb · 2018-04-17T20:25:58Z

src/core/Akka.Persistence/Eventsourced.Recovery.cs

-
-        public bool IsRecoveryRunning { get; }
-
+        public Func<bool> IsRecoveryRunning { get; }


Nah, this is an internal API. None of the third party Akka.Persistence plugins have access to this so we're able to preserve binary compatibility here. It's only the Akka.Persistence engine itself that consumes this directly so I think you're fine.

Aaronontheweb · 2018-04-17T20:26:43Z

src/core/Akka.Persistence/Eventsourced.Recovery.cs

            {
                try
                {
-                    if (message is ReplayedMessage)
+                    switch (message)


I like this change

Aaronontheweb · 2018-04-17T20:28:41Z

src/core/Akka.Persistence/Eventsourced.Recovery.cs

@@ -17,17 +17,15 @@ namespace Akka.Persistence

    internal class EventsourcedState
    {
-        public EventsourcedState(string name, bool isRecoveryRunning, StateReceive stateReceive)
+        public EventsourcedState(string name, Func<bool> isRecoveryRunning, StateReceive stateReceive)


Question: how often is the EventsourcedState object allocated? From a functional standpoint it should be fine, but if we allocate a new EventsourcedState instance regularly then anonymous method allocations could add up. I'm not familiar enough with Akka.Persistence's internals to know off-hand. Do you or @Horusiath have some idea?

Aaronontheweb · 2018-04-17T21:40:16Z

Left some review notes for you @ismaelhamed

ismaelhamed · 2018-04-19T17:27:01Z

src/core/Akka.Persistence.Tests/PersistentActorSpec.cs

+            persistentActor.Tell(GetState.Instance);
+            ExpectMsgInOrder("a-1", "a-2", "rc-1", "rc-2");
+            persistentActor.Tell(GetState.Instance);
+            ExpectMsgInOrder("a-1", "a-2", "rc-1", "rc-2", "rc-3");


There's some race condition (at least in the JVM) between the GetState and the PersistAsync, so they've simplified the test like this:

expectMsgAnyOf(List("a-1", "a-2", "rc-1", "rc-2"), List("a-1", "a-2", "rc-1", "rc-2", "rc-3"))

But I'm not sure this is possible with the current ExpectMsgAnyOf in Akka.NET

@Aaronontheweb about this?

I saw the comment - the test passes currently, right? If it's going to be racy, do we need to implement some new features in the TestKit in order to do this correctly? What's the issue?

The test runs fine, at least every time I've run it. But I noticed they (JVM) changed it since the original PR for the reason I mentioned, just so you know.

Aaronontheweb · 2018-06-07T15:20:37Z

Bleh, I forgot to come back and revisit this - will do

Aaronontheweb

Looks good to me. Just rebased on latest dev changes. Ready for merge when those specs come back.

Aaronontheweb · 2018-06-25T13:09:13Z

Thanks for your contribution @ismaelhamed

ismaelhamed commented Mar 17, 2018

View reviewed changes

ismaelhamed force-pushed the wip-persistence branch 2 times, most recently from 01f0463 to 6f0e47d Compare March 22, 2018 18:20

Danthar added the needs review label Mar 22, 2018

ismaelhamed force-pushed the wip-persistence branch 2 times, most recently from 9e35b77 to 22962d9 Compare March 26, 2018 16:58

ismaelhamed mentioned this pull request Apr 2, 2018

[WIP] Exception from ReceiveRecover should stop the PersistentActor #3374

Closed

ismaelhamed force-pushed the wip-persistence branch from 22962d9 to db12c06 Compare April 11, 2018 16:07

ismaelhamed force-pushed the wip-persistence branch from db12c06 to 97dcd79 Compare April 15, 2018 08:32

Aaronontheweb added the ready label Apr 17, 2018

ismaelhamed commented Apr 17, 2018

View reviewed changes

Aaronontheweb reviewed Apr 17, 2018

View reviewed changes

Aaronontheweb added reviewed with remarks and removed needs review labels Apr 17, 2018

ismaelhamed commented Apr 19, 2018

View reviewed changes

Aaronontheweb self-assigned this Jun 7, 2018

Aaronontheweb added this to the 1.3.9 milestone Jun 7, 2018

ismaelhamed force-pushed the wip-persistence branch 2 times, most recently from 68c50a3 to 5980c05 Compare June 10, 2018 11:05

ismaelhamed force-pushed the wip-persistence branch from 5980c05 to 79484cd Compare June 21, 2018 18:13

Aaronontheweb approved these changes Jun 22, 2018

View reviewed changes

Aaronontheweb added reviewed positively and removed reviewed with remarks labels Jun 22, 2018

ismaelhamed changed the title ~~[WIP] Allow persisting events when recovery has completed~~ Allow persisting events when recovery has completed Jun 22, 2018

Allow persisting events when recovery has completed

f574061

ismaelhamed force-pushed the wip-persistence branch from 334d899 to f574061 Compare June 23, 2018 07:46

Aaronontheweb merged commit fdc1765 into akkadotnet:dev Jun 25, 2018

ismaelhamed mentioned this pull request Jun 28, 2018

Reorder ChangeState(ProcessingCommands()) and OnReplaySuccess() inside Eventsourced.Recovery.cs #3472

Open

This was referenced Aug 22, 2018

Akka.NET v1.3.9 Release Notes #3574

Merged

Akka.NET v1.3.9 Production Release #3577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow persisting events when recovery has completed #3366

Allow persisting events when recovery has completed #3366

ismaelhamed commented Mar 17, 2018 •

edited

ismaelhamed Mar 17, 2018

Aaronontheweb Apr 17, 2018

ismaelhamed Apr 19, 2018

ismaelhamed Mar 17, 2018

Aaronontheweb commented Apr 14, 2018

ismaelhamed commented Apr 15, 2018

Aaronontheweb commented Apr 17, 2018

ismaelhamed Apr 17, 2018 •

edited

Aaronontheweb Apr 17, 2018

Aaronontheweb left a comment

Aaronontheweb Apr 17, 2018

ismaelhamed Apr 18, 2018 •

edited

Aaronontheweb Apr 17, 2018

Aaronontheweb Apr 17, 2018

Aaronontheweb Apr 17, 2018

Aaronontheweb commented Apr 17, 2018

ismaelhamed Apr 19, 2018

ismaelhamed Jun 22, 2018

Aaronontheweb Jun 22, 2018

ismaelhamed Jun 23, 2018 •

edited

Aaronontheweb commented Jun 7, 2018

Aaronontheweb left a comment

Aaronontheweb commented Jun 25, 2018


		return new EventsourcedState("replay started", true, (receive, message) =>
		return new EventsourcedState("replay started", () => recoveryRunning, (receive, message) =>


		public bool IsRecoveryRunning { get; }

		public Func<bool> IsRecoveryRunning { get; }

Allow persisting events when recovery has completed #3366

Allow persisting events when recovery has completed #3366

Conversation

ismaelhamed commented Mar 17, 2018 • edited

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

Aaronontheweb commented Apr 14, 2018

ismaelhamed commented Apr 15, 2018

Aaronontheweb commented Apr 17, 2018

ismaelhamed Apr 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelhamed Apr 18, 2018 • edited

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

Aaronontheweb commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelhamed Jun 23, 2018 • edited

Choose a reason for hiding this comment

Aaronontheweb commented Jun 7, 2018

Aaronontheweb left a comment

Choose a reason for hiding this comment

Aaronontheweb commented Jun 25, 2018

ismaelhamed commented Mar 17, 2018 •

edited

ismaelhamed Apr 17, 2018 •

edited

ismaelhamed Apr 18, 2018 •

edited

ismaelhamed Jun 23, 2018 •

edited