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

Refactoring CoordinatedShutdownProvider object #8532

Merged
merged 3 commits into from Jul 24, 2018

Conversation

Projects
None yet
3 participants
@marcospereira
Copy link
Member

marcospereira commented Jul 23, 2018

Purpose

Two main changes here:

  1. Users should interact directly with Akka's CoordinatedShutdown API instead of relying on Play helpers. This way evolutions on Akka API will be available faster and more consistently.
  2. Renaming from CoordinatedShutdownProvider to CoordinatedShutdownSupport.

This may later have some impact on Lagom since it depending on these APIs (at least this is the direction in lagom/lagom#1453. What we can do is to access Play's CoordinatedShutdownSupport just like it was done in TrampolineContextAccessor.scala.

@marcospereira marcospereira changed the title Mark CoordinatedShutdownProvider APIs as private Refactoring CoordinatedShutdownProvider object Jul 23, 2018

@ignasi35
Copy link
Member

ignasi35 left a comment

LGTM.

Users should only use CoordinatedShutdown's API to add tasks. Invoking the execution of the shutdown will unlikely be user code; generally it's a JVM hook, a cluster downing or some other framework code.

marcospereira added some commits Jul 23, 2018

Mark CoordinatedShutdownProvider APIs as private
Users should interact directly with Akka's CoordinatedShutdown API instead
of relying on Play helpers. This way evolutions on Akka API will be available
faster and in a more consistent way.

@marcospereira marcospereira force-pushed the marcospereira:coordinated-shutdown-private-apis branch from 77e88b2 to 69c0cc4 Jul 24, 2018

@marcospereira

This comment has been minimized.

Copy link
Member

marcospereira commented Jul 24, 2018

Fixed conflicts.

Invoking the execution of the shutdown will unlikely be user code

And even if they want/need to, it is better if they use Akka APIs directly instead. No need to provide a wrapper in Play. :-)

@ignasi35 ignasi35 merged commit baf2be3 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-private-apis branch Jul 24, 2018

@TimMoore

This comment has been minimized.

Copy link
Contributor

TimMoore commented Jul 30, 2018

This may later have some impact on Lagom since it depending on these APIs (at least this is the direction in lagom/lagom#1453. What we can do is to access Play's CoordinatedShutdownSupport just like it was done in TrampolineContextAccessor.scala.

I think I'd rather copy the syncShutdown implementation into Lagom… the workaround in TrampolineContextAccessor is pretty complicated and ugly, and the code in syncShutdown is pretty simple now that the configuration to run from a specific phase has been removed.

Another point is that maybe it's not so great that Lagom only provides blocking methods for shutting down LagomClientFactory and LagomClientApplication. Maybe non-blocking alternatives should be provided and the blocking ones deprecated.

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