Skip to content

Better recovery timeout for persistent actors #20738 - #20753

Merged
johanandren merged 5 commits into
akka:masterfrom
johanandren:wip-20738-setReceiveTimeout-interference-persistence-johanandren
Jun 9, 2016
Merged

johanandren merged 5 commits into
akka:masterfrom
johanandren:wip-20738-setReceiveTimeout-interference-persistence-johanandren

Conversation

@johanandren

Copy link
Copy Markdown
Contributor

Fixes #20738 by using the scheduler instead of receiveTimeout additionally it will therefore not be affected by non-recovery messages passing by into the stash of the persistent actor.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins labels Jun 8, 2016
@akka-ci

akka-ci commented Jun 8, 2016

Copy link
Copy Markdown

Test PASSed.

@akka-ci akka-ci removed the validating PR is currently being validated by Jenkins label Jun 8, 2016
new State {

// protect against snapshot stalling forever because of journal overloaded and such
val timeout = extension.journalConfigFor(journalPluginId).getMillisDuration("recovery-event-timeout")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps pass as a param, to avoid reading the config twice

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could I even put it in a place so it doesn't have to be read once per persistent actor start?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't put it in the trait because that would break bin compat.
We could change journalConfigFor to return a Settings object that would read such things once, but I don't think it's worth it. It's rather costly to start a persistent actor anyway.

}
case RecoverySuccess(highestSeqNr) ⇒
resetRecieveTimeout()
timeoutCancellable.cancel()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a small chance that a RecoveryTick is already enqueued in the mailbox here and will be delivered to user's receive. That could be filtered out innProcessingState or at least ignored in unhandled.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, I was going to ask about that. Will look into it.

@patriknw

patriknw commented Jun 8, 2016

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment
@drewhk

drewhk commented Jun 8, 2016

Copy link
Copy Markdown
Contributor

LGTM


case _: RecoveryTick =>
// we may have one of these scheduled before the scheduled timeout
// is cancelled, just consume it so the concrete actor never sees it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was weird phrasing, will redo

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jun 9, 2016
@akka-ci

akka-ci commented Jun 9, 2016

Copy link
Copy Markdown

Test PASSed.

private final case class AsyncHandlerInvocation(evt: Any, handler: Any ⇒ Unit) extends PendingHandlerInvocation

/** message used to detect that recovery timed out */
private case class RecoveryTick(snapshot: Boolean)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

@patriknw

patriknw commented Jun 9, 2016

Copy link
Copy Markdown
Contributor

LGTM

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jun 9, 2016
@akka-ci

akka-ci commented Jun 9, 2016

Copy link
Copy Markdown

Test PASSed.

@johanandren
johanandren merged commit 16cde39 into akka:master Jun 9, 2016
zbynek001 added a commit to zbynek001/akka.net that referenced this pull request Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tested PR that was successfully built and tested by Jenkins

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants