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

Remove `play.akka.run-cs-from-phase` configuration #8531

Merged
merged 3 commits into from Jul 24, 2018

Conversation

Projects
None yet
4 participants
@marcospereira
Copy link
Member

marcospereira commented Jul 23, 2018

Purpose

The default now is to run all the phases. Users can create an instance of CoordinatedShutdown and run from a specific phase if they want/need to.

Remove `play.akka.run-cs-from-phase` configuration
The default now is to run all the phases. Users can create an instance of
CoordinatedShutdown and run from a specific phase it they want/need to.
@richdougherty
Copy link
Member

richdougherty left a comment

A couple of minor changes.

Try(actorSystem.settings.config.getString("play.akka.run-cs-from-phase"))
.toOption
.filterNot(_.isEmpty)
private[play] lazy val logger = LoggerFactory.getLogger(classOf[CoordinatedShutdownProvider])

This comment has been minimized.

@richdougherty

richdougherty Jul 23, 2018

Member

Out of interest, is there a reason not to make this an eager val?

This comment has been minimized.

@marcospereira

marcospereira Jul 23, 2018

Member

Nah, it is just that maybe the logger is not used at all.

This comment has been minimized.

@richdougherty

richdougherty Jul 23, 2018

Member

OK, fine either way.


val akkaTimeoutKey = "akka.coordinated-shutdown.phases.actor-system-terminate.timeout"
val playTimeoutKey = "play.akka.shutdown-timeout"

"ActorSystemProvider" should {

"use Play's 'play.akka.shutdown-timeout' if defined " in {
withOverridenTimeout {
withOverriddenTimeout {

This comment has been minimized.

@richdougherty

richdougherty Jul 23, 2018

Member

👍 😂

phaseClusterExitingDoneExecuted.get() must equalTo(true)
phaseClusterShutdownExecuted.get() must equalTo(true)
phaseBeforeActorSystemTerminateExecuted.get() must equalTo(true)
phaseActorSystemTerminateExecuted.get() must equalTo(true)

This comment has been minimized.

@richdougherty

richdougherty Jul 23, 2018

Member

Impressive! What about something like this:

class PhaseTest(val phase) {
  def name = phase.getClass.getName
  val called = AtomicBoolean(false)
}

val phaseTests: Seq[PhaseTest] = Seq(
  PhaseBeforeServiceUnbind,
  PhaseServiceUnbind,
  PhaseServiceRequestsDone,
  ...
).map(p => new PhaseTest(p))

phaseTests.foreach { pt =>
  cs.addTask(pt.phase, "test-${pt.name}") {
    val before, after = paseTests.span(_.phase != pt.phase)
    before.map(_.called.get) must contain(beTrue).forall
    after.map(_.called.get) must contain(beFalse).forall
    pt.called.set(true)
  }
}

phasesTests.forEach { pt => pt.called.get must beTrue }

This comment has been minimized.

@marcospereira

marcospereira Jul 23, 2018

Member

I thought about it, but it would be a similar amount of code, and maybe the test would be less direct? I considered that it was okay to have more code here if the test has less logic.

Anyway, maybe I just need the first and last phases here since they form a DAG.

This comment has been minimized.

@marcospereira

This comment has been minimized.

@richdougherty

richdougherty Jul 23, 2018

Member

As minimal as possible. I like it!


## CoordinatedShutdown `play.akka.run-cs-from-phase` configuration

The configuration `play.akka.run-cs-from-phase` is not supported anymore and adding it has no effect. A warning is logged if it is present. Play now run all the phases to ensure that all hooks registered in `ApplicationLifecycle` or all the tasks added to coordinated shutdown are executed. If you need to run `CoordinatedShutdown` from a specific phase, you can always do it manually:

This comment has been minimized.

@richdougherty

richdougherty Jul 23, 2018

Member

Typo: Play now run -> Play now runs

"and" instead of "or"? all hooks registered in ApplicationLifecycle or all the tasks added to coordinated shutdown -> all hooks registered in ApplicationLifecycle and all the tasks added to coordinated shutdown

This comment has been minimized.

@marcospereira

marcospereira Jul 23, 2018

Member

Fixed. ;-)

Thanks.

marcospereira added some commits Jul 23, 2018

@richdougherty
Copy link
Member

richdougherty left a comment

👍

The configuration `play.akka.run-cs-from-phase` is not supported anymore and adding it has no effect. A warning is logged if it is present. Play now runs all the phases to ensure that all hooks registered in `ApplicationLifecycle` and all the tasks added to coordinated shutdown are executed. If you need to run `CoordinatedShutdown` from a specific phase, you can always do it manually:

```scala
val reason = CoordinatedShutdown.UnknownReason

This comment has been minimized.

@ignasi35

ignasi35 Jul 24, 2018

Member

(nitpick) don't suggest using UnknownReason. Not running all phases is an edge case that probably has a very meaningful reason.

This comment has been minimized.

@ignasi35

ignasi35 Jul 24, 2018

Member

What I'm trying to say is that it's better to inform users about the importance of using meaningful Reason's.

This comment has been minimized.

@ignasi35

ignasi35 Jul 24, 2018

Member

We can do this in a separate PR

This comment has been minimized.

@TimMoore

TimMoore Jul 30, 2018

Contributor

@ignasi35 could you raise an issue (if you haven't already) and link it here? Thanks!

This comment has been minimized.

@ignasi35

ignasi35 Oct 31, 2018

Member

just noticed I never did this. Issue: #8751

if (config.hasPath("play.akka.run-cs-from-phase")) {
logger.warn("Configuration 'play.akka.run-cs-from-phase' was deprecated and has no effect. Play now run all the CoordinatedShutdown phases.")
}
}

This comment has been minimized.

@ignasi35

@ignasi35 ignasi35 merged commit 835b2ab into playframework:master Jul 24, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@marcospereira marcospereira deleted the marcospereira:coordinated-shutdown-runAll branch Jul 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment