Skip to content
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

Use system.registerOnTermination instead of Coordinated shutdown #193

Merged
merged 3 commits into from Jul 23, 2018

Conversation

@WellingR
Copy link
Member

WellingR commented Jun 5, 2018

Fixes #192

I have removed all usages of and references to coordinates shutdown and replaced them with system.registerOnTermination.

The existing configuration to add a shutdown hook is still used. So it is still possible to disable this functionality

@WellingR WellingR requested review from renatocaval and ignasi35 Jun 6, 2018
Copy link
Member

ignasi35 left a comment

LGTM

@WellingR WellingR changed the title [WIP] Use system.registerOnTermination instead of Coordinated shutdown Use system.registerOnTermination instead of Coordinated shutdown Jun 6, 2018
Copy link
Member

renatocaval left a comment

@WellingR, I think there are some leftovers from the previous implementation.

Here is what I see:

  • SlickExtensions, always creates the db being it shared or not
  • each component ask for the DB and each component need to read config to infer if it must close the DB or not

Here is my suggestion:

  • SlickExtension.database should return a tuple (Database, Boolean)
  • the Boolean signals to the component if it must close it or not
  • DbHolder doesn't need to have an addShutDownHook boolean. The rule being, if there is a DbHolder, it's shared and therefore we need to close it on system shutdown, not the component. Btw, it's now not even using that boolean.

If we implement like that we have fewer configurations and fewer places to infer what we should do. And the equation becomes, is shared, sys.shutdown, not shared component takes over.

WDYT?

@renatocaval

This comment has been minimized.

Copy link
Member

renatocaval commented Jun 8, 2018

A second thought on my last comment...

When storing the DB reference in JNDI (Lagom's case), the DB is shared, but the plugin makes no distinction from a non-shared one.

We don't need to add it on this PR, but we could also use a DbHolder for the JNDI Db. In which case, when shutting down the actor system, it will only be closed once and not per component.

/cc @ignasi35

@WellingR

This comment has been minimized.

Copy link
Member Author

WellingR commented Jun 8, 2018

@renatocaval

SlickExtensions, always creates the db being it shared or not

Not completely true: the DbHolder for a shared database is created when the default SlickExtension is loaded. The DbHolder uses a lazy val to ensure that the database is created at most once.
Non-shared database are always created when requested.

Concerning your suggestion

DbHolder doesn't need to have an addShutDownHook boolean. The rule being, if there is a DbHolder, it's shared and therefore we need to close it on system shutdown, not the component.

Does this mean you want to drop the shared-db-add-shutdown-hook setting? I actually agree that it probably makes sense to always shut down a shared database, if a user wants to shutdown the database manually it is still possible do configure a custom implementation of the slickextension.

When storing the DB reference in JNDI (Lagom's case), the DB is shared, but the plugin makes no distinction from a non-shared one.
We don't need to add it on this PR, but we could also use a DbHolder for the JNDI Db. In which case, when shutting down the actor system, it will only be closed once and not per component.

Actually all we need to do is to document that JNDI databases should be configured as shared databases. Perhaps we could even log a warning if JNDI is used in a non-shared database.

@renatocaval

This comment has been minimized.

Copy link
Member

renatocaval commented Jun 10, 2018

@WellingR

SlickExtensions, always creates the db being it shared or not

What meant by that is that creation is managed by the extension. It's the extension that always make the decision. For instance, it makes the decision that a Db will be created lazily by the DbHolder. It can be the one holding all the necessary information and signaling to the other parts how they should behave.

Does this mean you want to drop the shared-db-add-shutdown-hook setting?

Yes, I think it's not needed anymore and users in need of a more fine grained control can impl their own variant.


With respect to the JNDI case, I don't know what is the best approach.
In the specific case of Lagom, the JNDI Db is configured somewhere else. When the plugin bootstraps and finds nothing on the normal config path, it tries a lookup in JNDI. At that moment, the JNDI Db was already created and it's waiting for the plugin to come pick it.

In that case, it's not possible to configure it as a shared Db, because it will be wrapped by a DbHolder but nobody will add it to the JNDI.

My initial thought was to detect that we were using a JNDI and treat is as a shared Db. We can make the extension aware that it's using something from JNDI. In that case, it returns the JNDI Db, but it register a shutdown hook for it (like with shared db). And it signals to the components that they should not close the Db if they crash for some reason.

Or even better, if we detect that we are using JNDI, we tell the component to NOT close it, but the we DON'T register a shutdown hook neither. It's the one that created the Db and register it in JNDI the responsibility to close it, not the plugin. The plugin is just consuming a JNDI resource, it's not responsible to managing it.

We can leave the JNDI case on the side for now. Let's think more about it and let's not block this current PR because of it.

@WellingR

This comment has been minimized.

Copy link
Member Author

WellingR commented Jun 19, 2018

@renatocaval I have implemented the changes you suggested. Can you have a look?

def allowShutdown: Boolean
}

case class StrictSlickDatabase(database: Database, profile: JdbcProfile) extends SlickDatabase {

This comment has been minimized.

Copy link
@WellingR

WellingR Jun 19, 2018

Author Member

I am not really sure if this is the best name for the "simple" version if the SlickDatabase. Any suggestions?

This comment has been minimized.

Copy link
@renatocaval

renatocaval Jul 10, 2018

Member

Maybe we should call it EagerSlickDatabase instead as opposed to the lazy one.

@WellingR

This comment has been minimized.

Copy link
Member Author

WellingR commented Jun 19, 2018

By the way, a disadvantage of the updated implementation is that it breaks the SlickExtension interface which we released in the previous version.

Copy link
Member

renatocaval left a comment

LGTM, thanks for implementing the changes we discussed. I left some small comments.

This will have some impact on Lagom, positive impact.

We will need to have our own SlickDatabaseProvider that to do the lookup in JNDI and to register it to shut down, only once, when the actor system shuts down. Probably using coordinated shutdown.

That's a missing bit we have today, we create the DB in JNDI, but the plugin components close it in an indeterministic fashion.
(cc @ignasi35 @TimMoore)

def allowShutdown: Boolean
}

case class StrictSlickDatabase(database: Database, profile: JdbcProfile) extends SlickDatabase {

This comment has been minimized.

Copy link
@renatocaval

renatocaval Jul 10, 2018

Member

Maybe we should call it EagerSlickDatabase instead as opposed to the lazy one.

def profile: JdbcProfile

/**
* If true, the requesting side usualy a (read/write/snapshot journal)

This comment has been minimized.

Copy link
@renatocaval

renatocaval Jul 10, 2018

Member

typo: usually

@WellingR

This comment has been minimized.

Copy link
Member Author

WellingR commented Jul 22, 2018

@renatocaval I resolved the final comments you had. Can this be merged?

@renatocaval

This comment has been minimized.

Copy link
Member

renatocaval commented Jul 22, 2018

@WellingR WellingR merged commit 198a48b into master Jul 23, 2018
3 checks passed
3 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@WellingR WellingR deleted the do-not-use-coordinated-shutdown branch Jul 23, 2018
@WellingR WellingR added this to the 3.5.0 milestone Sep 7, 2018
evgenyzic added a commit to evgenyzic/akka-persistence-jdbc that referenced this pull request Dec 16, 2019
…a#193)

Use system.registerOnTermination instead of Coordinated shutdown to close the database
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.