-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow persisting events when recovery has completed #21893
Conversation
Can one of the repo owners verify this patch? |
else { | ||
changeState(processingCommands) | ||
internalStash.unstashAll() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, that makes sense
class PersistInRecovery(name: String) extends ExamplePersistentActor(name) { | ||
override def receiveRecover = super.receiveRecover orElse { | ||
case RecoveryCompleted ⇒ | ||
persist(Evt(RecoveryCompleted))(updateState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if persist
is done when processing events in receiveRecover
, i.e. not only when RecoveryCompleted
?
class PersistInRecovery(name: String) extends ExamplePersistentActor(name) { | ||
override def receiveRecover = super.receiveRecover orElse { | ||
case RecoveryCompleted ⇒ | ||
persist(Evt(RecoveryCompleted))(updateState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would also be interesting with a test that is using persistAsync
from receiveRecover
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -43,7 +43,7 @@ trait AsyncWriteJournal extends Actor with WriteJournalBase with AsyncRecovery { | |||
case "fail" ⇒ ReplayFilter.Fail | |||
case "warn" ⇒ ReplayFilter.Warn | |||
case other ⇒ throw new IllegalArgumentException( | |||
s"invalid replay-filter.mode [$other], supported values [off, repair, fail, warn]") | |||
s"invalid replay-filter.mode [$other], supported values [off, repair-by-discard-old, fail, warn]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
OK TO TEST |
Test PASSed. |
@@ -316,6 +316,7 @@ private[persistence] trait Eventsourced extends Snapshotter with PersistenceStas | |||
* @param handler handler for each persisted `event` | |||
*/ | |||
def persist[A](event: A)(handler: A ⇒ Unit): Unit = { | |||
if (recoveryRunning) throw new IllegalStateException("Cannot persist event during recovery") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the whole point to be able to persist events during recovery? I am confused here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too cannot get this to match the name of the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that "Cannot persist event during recovery" message is not descriptive. Could you suggest suitable one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot persist during replay. Events can be persisted when receiving RecoveryCompleted or later.
expectMsg(List("a-1", "a-2", "rc-1", "rc-2", "rc-3", "invalid")) | ||
watch(persistentActor) | ||
persistentActor ! "boom" | ||
expectTerminated(persistentActor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it is actually fail-fast on persist during recovery, but allow it in RecoveryCompleted
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Test PASSed. |
LGTM, thanks - nice improvement! |
Refs #21736