You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@helix.apache.org by liyinan926 <gi...@git.apache.org> on 2015/09/09 02:16:46 UTC
[GitHub] helix pull request: Makes TaskStateModel gets properly shutdown wh...
GitHub user liyinan926 opened a pull request:
https://github.com/apache/helix/pull/35
Makes TaskStateModel gets properly shutdown when the JVM exists
`TaskStateModel.shutdown()` is not called anywhere in the code base. This PR adds a `ShutdownHook` to properly shutdown each `TaskStateModel` instance.
Signed-off-by: Yinan Li <li...@gmail.com>
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/liyinan926/helix task-state-model-shutdown
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/helix/pull/35.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #35
----
commit 75e7ecbe63eb5a402f3d3987364967535f00e096
Author: Yinan Li <li...@gmail.com>
Date: 2015-09-09T00:14:55Z
Makes TaskStateModel gets properly shutdown when the JVM exists
Signed-off-by: Yinan Li <li...@gmail.com>
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: Makes TaskStateModel gets properly shutdown wh...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on the pull request:
https://github.com/apache/helix/pull/35#issuecomment-138738054
@kishoreg Can you take a look at this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-610] Makes sure TaskStateModel gets pro...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on the pull request:
https://github.com/apache/helix/pull/35#issuecomment-139025101
@kishoreg Updated 6ae7891.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-610] Makes sure TaskStateModel gets pro...
Posted by kishoreg <gi...@git.apache.org>.
Github user kishoreg commented on the pull request:
https://github.com/apache/helix/pull/35#issuecomment-138981788
@liyinan926 https://github.com/apache/helix/blob/master/helix-core/src/main/java/org/apache/helix/api/StateTransitionHandlerFactory.java If you look at the methods here, you should be able to get references to all TransitionHandlers.
getResourceSet and then for each resource get PartitionSet and for each partition get the transition handler. You can then call shutdown on the transition handler.
Note that I agree with you that we need a way to gracefully shutdown all transition handlers but it would be great to handle this in one place and make it common to all transition handlers rather than making it specific to taskstatemodel. Do you think you can extend this change to all transition handler? Note that with this we only need to have a shutdownhook in helixstatemachineengine and we should be able to get references to all statemodels.
This will also slow down the shutdown process depending on whats done in shutdown method. May be we can use a marker interface to indicate that shutdown method should be invoked for these handlers
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-610] Makes sure TaskStateModel gets pro...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on the pull request:
https://github.com/apache/helix/pull/35#issuecomment-139339504
@kishoreg Can you take a look at this PR again? Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-610] Makes sure TaskStateModel gets pro...
Posted by kishoreg <gi...@git.apache.org>.
Github user kishoreg commented on the pull request:
https://github.com/apache/helix/pull/35#issuecomment-138792278
This is an interesting change, why cant we do this on the client side? If you have the reference to StateModelFactory, you can call the shutdown method on each and every instance of statemodel
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-610] Makes sure TaskStateModel gets pro...
Posted by kishoreg <gi...@git.apache.org>.
Github user kishoreg commented on the pull request:
https://github.com/apache/helix/pull/35#issuecomment-139423489
ok LGTM, ideally I would have preferred this code to be in HelixStateMachine. I will merge it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-610] Makes sure TaskStateModel gets pro...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on the pull request:
https://github.com/apache/helix/pull/35#issuecomment-138976620
@kishoreg I do have a reference to a `TaskStateModelFactory` instance, but I don't have reference to any `TaskStateModel` instances created by it though so I cannot call shutdown directly. It also seems the only place `TaskStateModelFactory.createStateTransitionHandler` is called is in `StateTransitionHandlerFactory.createAndAddSTransitionHandler`, which doesn't know about the `shutdown` method specific to `TaskStateModel`. I think what I can do is to add a `shutdown` method to `TaskStateModelFactory` and store references to all created `TaskStateModel` instances in the `TaskStateModelFactory` and shutdown all of them in `TaskStateModelFactory.shutdown`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-610] Makes sure TaskStateModel gets pro...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/helix/pull/35
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-610] Makes sure TaskStateModel gets pro...
Posted by liyinan926 <gi...@git.apache.org>.
Github user liyinan926 commented on the pull request:
https://github.com/apache/helix/pull/35#issuecomment-139021356
@kishoreg Yes, I was actually looking at `StateTransitionHandlerFactory` and I thought about doing that. The problem is `shutdown` is specific to `TaskStateModel` but not to the general `TransitionHandler`. So in order to do that in `StateTransitionHandlerFactory` I need to cast `TransitionHandler` to `TaskStateModel` whenever appropriate, which is hard to do.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---