You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by Parth-Brahmbhatt <gi...@git.apache.org> on 2014/06/16 23:14:17 UTC
[GitHub] incubator-storm pull request: replacing RunTime.halt() with RunTim...
GitHub user Parth-Brahmbhatt opened a pull request:
https://github.com/apache/incubator-storm/pull/143
replacing RunTime.halt() with RunTime.exit() with a special shutdown hoo...
replacing RunTime.halt() with RunTime.exit() with a special shutdown hook that allows 1 second for cleanup shutdown hooks and then sends kill -9 to process. Added shutdown hooks for supervisor and worker.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Parth-Brahmbhatt/incubator-storm STORM-183
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-storm/pull/143.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 #143
----
commit 2c2570fcd707b71d69db81736ba703707cee3ae7
Author: Parth Brahmbhatt <br...@gmail.com>
Date: 2014-06-16T21:06:05Z
replacing RunTime.halt() with RunTime.exit() with a special shutdown hook that allows 1 second for cleanup shutdown hooks and then sends kill -9 to process. Added shutdown hooks for supervisor and worker.
----
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46615057
Thanks for the update. It looks good, but a few minor things to rearrange. Thanks for sticking with this it will be a huge benefit to storm once it is in.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46245056
I have also hit a few situations where the kill -9 never came out. I saw the kill -15 but not the final death blow. I can reproduce it by launching several word count applications on a 1 node cluster, and then killing them 1 at a time (whichever one has the full 3 workers running) Eventually I am left with several workers that refuse to die.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/143#discussion_r13833623
--- Diff: storm-core/src/clj/backtype/storm/util.clj ---
@@ -388,13 +388,51 @@
(catch IOException e
(log-message "Could not extract " dir " from " jarpath))))
-(defn ensure-process-killed! [pid]
+(defn sleep-secs [secs]
+ (when (pos? secs)
+ (Time/sleep (* (long secs) 1000))))
+
+(defn sleep-until-secs [target-secs]
+ (Time/sleepUntil (* (long target-secs) 1000)))
+
+(def ^:const sig-kill 9)
+
+(def ^:const sig-term 15)
+
+(defn send-signal-to-process
+ [pid signum]
+ (try-cause
+ (exec-command! (str (if on-windows?
+ (if (== signum sig-kill) "taskkill /f /pid " "taskkill /pid ")
+ (str "kill -" signum " "))
+ pid))
+ (catch ExecuteException e
+ (log-message "Error when trying to kill " pid ". Process is probably already dead."))))
+
+(defn force-kill-process
+ [pid]
+ (send-signal-to-process pid sig-kill))
+
+(defn kill-process-with-sig-term
+ [pid]
+ (send-signal-to-process pid sig-term))
+
+(defn ensure-process-killed!
+ [pid]
;; TODO: should probably do a ps ax of some sort to make sure it was killed
(try-cause
- (exec-command! (str (if on-windows? "taskkill /f /pid " "kill -9 ") pid))
+ (kill-process-with-sig-term pid)
(catch ExecuteException e
(log-message "Error when trying to kill " pid ". Process is probably already dead."))))
+(defn add-shutdown-hook-with-force-kill-in-1-sec
+ "adds the user supplied function as a shutdown hook for cleanup.
+ Also adds a function that sleeps for a second and then sends kill -9 to process to avoid any zombie process in case
+ cleanup function hangs."
+ [func]
+ (.addShutdownHook (Runtime/getRuntime) (Thread. #((func)
+ (sleep-secs 1)
+ (force-kill-process (process-pid))))))
--- End diff --
done.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46622640
Thanks for clarifying that, I forgot that is what it was doing. Looking forward to your update reordering the supervisor cleanup order.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-47037071
+1
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46732024
I am a +1. Everything seems to be working OK.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/143#discussion_r13833622
--- Diff: storm-core/src/clj/backtype/storm/util.clj ---
@@ -388,13 +388,51 @@
(catch IOException e
(log-message "Could not extract " dir " from " jarpath))))
-(defn ensure-process-killed! [pid]
+(defn sleep-secs [secs]
+ (when (pos? secs)
+ (Time/sleep (* (long secs) 1000))))
+
+(defn sleep-until-secs [target-secs]
+ (Time/sleepUntil (* (long target-secs) 1000)))
+
+(def ^:const sig-kill 9)
+
+(def ^:const sig-term 15)
+
+(defn send-signal-to-process
+ [pid signum]
+ (try-cause
+ (exec-command! (str (if on-windows?
+ (if (== signum sig-kill) "taskkill /f /pid " "taskkill /pid ")
+ (str "kill -" signum " "))
+ pid))
+ (catch ExecuteException e
+ (log-message "Error when trying to kill " pid ". Process is probably already dead."))))
+
+(defn force-kill-process
+ [pid]
+ (send-signal-to-process pid sig-kill))
+
+(defn kill-process-with-sig-term
+ [pid]
+ (send-signal-to-process pid sig-term))
+
+(defn ensure-process-killed!
+ [pid]
;; TODO: should probably do a ps ax of some sort to make sure it was killed
(try-cause
- (exec-command! (str (if on-windows? "taskkill /f /pid " "kill -9 ") pid))
+ (kill-process-with-sig-term pid)
(catch ExecuteException e
(log-message "Error when trying to kill " pid ". Process is probably already dead."))))
+(defn add-shutdown-hook-with-force-kill-in-1-sec
--- End diff --
You are right, I want to avoid a model where people have to call (.addShutdownHook blah blah) and then remember to call (add-shutdown-hook-with-force-kill-in-1-sec).
How about adding the use function and the actual forcing function as two seperate shutdown hook threads?
(defn add-shutdown-hook-with-force-kill-in-1-sec
[func]
(.addShutdownHook (Runtime/getRuntime) (Thread. #(func)))
(.addShutdownHook (Runtime/getRuntime) (Thread. #((sleep-secs 1)
(.halt (RunTime/getRunTime) 20)))))
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46248380
When I ran I did run jstack on the workers and all of them were stuck somewhere in the shutdown. So if you fixed the worker you would not see this.
```"Thread-23" #51 prio=5 os_prio=31 tid=0x00007f91a1059000 nid=0xb103 in Object.wait() [0x0000000193600000]
java.lang.Thread.State: WAITING (on object monitor)
at java.lang.Object.wait(Native Method)
at java.lang.Thread.join(Thread.java:1244)
- locked <0x000000011a65d4f8> (a java.lang.Thread)
at java.lang.Thread.join(Thread.java:1318)
at backtype.storm.util$async_loop$reify__1051.join(util.clj:486)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:483)
at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:93)
at clojure.lang.Reflector.invokeNoArgInstanceMember(Reflector.java:298)
at backtype.storm.daemon.executor$mk_executor$reify__5504.shutdown(executor.clj:353)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:483)
at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:93)
at clojure.lang.Reflector.invokeNoArgInstanceMember(Reflector.java:298)
at backtype.storm.daemon.worker$fn__5930$exec_fn__2806__auto____5931$shutdown_STAR___5949.invoke(worker.clj:392)
at backtype.storm.daemon.worker$fn__5930$exec_fn__2806__auto__$reify__5975.shutdown(worker.clj:423)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:483)
at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:93)
at clojure.lang.Reflector.invokeNoArgInstanceMember(Reflector.java:298)
at backtype.storm.daemon.worker$_main$fn__6002.invoke(worker.clj:455)
at backtype.storm.util$add_shutdown_hook_with_force_kill_in_1_sec$fn__975.invoke(util.clj:433)
at clojure.lang.AFn.run(AFn.java:24)
at java.lang.Thread.run(Thread.java:744)```
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-47103756
@Parth-Brahmbhatt I am going to merge this in, but I want to make sure I give the proper credit, both in the README and on the JIRA. For now I am just including your github account, but if you want me to assign the JIRA to you and update the README please let me know which JIRA account to use.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-storm/pull/143
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46250710
I have tried with both old and new code and I still can not reproduce this. I have tried killing the process with both SIGTERM and SIGINT, in both cases the process terminates.
If you are killing in some other way or if you have some other local code change to make workers sleep during shutdown phase then let me know.
I have checked in the change to make the final shutdown blow run in a separate thread.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46302266
I do like combining the two shutdown hooks together.
I understand what is happening now with the leaked processes now. I was confused about exactly what the code was supposed to be doing.
I thought that the supervisor would send a sig term to the process, and then wait a while and send a sig kill to it. I didn't realize that instead it was just sending a sigterm, and letting the worker send the sigkill to itself.
The issue with not having the supervisor force-kill the child is that a bug in the worker, or in a child process the worker forks, could result in the process being leaked.
I don't want to do the sleep right after the sigterm, because if there are multiple workers the sleeps will add up. I think we want to modify sync-processes in the supervisor to do 2 passes over the workers that need to be killed. The first pass would ask the worker to exit (sigterm). The second pass would force-kill the worker and cleanup the directories associated with it. There could be a 1 second sleep in between if any workers were being killed (I don't want to sleep if no workers are shutting down).
Does that sound reasonable?
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46333200
Not sure why a bug in the worker would cause leaked processes even when we already have a shutdown hook in workers that should kill it in a second but I am no expert on unix signal processing so I will trust you on that. Added the supervisor sequence to do what you have proposed.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46246787
i am trying to reproduce this but in my case the worker always ends up killing, I am testing with the new change where the force-shutdown runs in a separate thread so it is not blocked by the other shutdown hook function. How r u killing the workers? Did you make any other code changes locally to ensure workers hang around longer?
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/143#discussion_r13833188
--- Diff: storm-core/src/clj/backtype/storm/util.clj ---
@@ -388,13 +388,51 @@
(catch IOException e
(log-message "Could not extract " dir " from " jarpath))))
-(defn ensure-process-killed! [pid]
+(defn sleep-secs [secs]
+ (when (pos? secs)
+ (Time/sleep (* (long secs) 1000))))
+
+(defn sleep-until-secs [target-secs]
+ (Time/sleepUntil (* (long target-secs) 1000)))
+
+(def ^:const sig-kill 9)
+
+(def ^:const sig-term 15)
+
+(defn send-signal-to-process
+ [pid signum]
+ (try-cause
+ (exec-command! (str (if on-windows?
+ (if (== signum sig-kill) "taskkill /f /pid " "taskkill /pid ")
+ (str "kill -" signum " "))
+ pid))
+ (catch ExecuteException e
+ (log-message "Error when trying to kill " pid ". Process is probably already dead."))))
+
+(defn force-kill-process
+ [pid]
+ (send-signal-to-process pid sig-kill))
+
+(defn kill-process-with-sig-term
+ [pid]
+ (send-signal-to-process pid sig-term))
+
+(defn ensure-process-killed!
+ [pid]
;; TODO: should probably do a ps ax of some sort to make sure it was killed
(try-cause
- (exec-command! (str (if on-windows? "taskkill /f /pid " "kill -9 ") pid))
+ (kill-process-with-sig-term pid)
(catch ExecuteException e
(log-message "Error when trying to kill " pid ". Process is probably already dead."))))
+(defn add-shutdown-hook-with-force-kill-in-1-sec
+ "adds the user supplied function as a shutdown hook for cleanup.
+ Also adds a function that sleeps for a second and then sends kill -9 to process to avoid any zombie process in case
+ cleanup function hangs."
+ [func]
+ (.addShutdownHook (Runtime/getRuntime) (Thread. #((func)
+ (sleep-secs 1)
+ (force-kill-process (process-pid))))))
--- End diff --
I would prefer to do a Runtime.halt here instead of force-kill-process. I think it is more future proof.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/143#discussion_r13833165
--- Diff: storm-core/src/clj/backtype/storm/util.clj ---
@@ -388,13 +388,51 @@
(catch IOException e
(log-message "Could not extract " dir " from " jarpath))))
-(defn ensure-process-killed! [pid]
+(defn sleep-secs [secs]
+ (when (pos? secs)
+ (Time/sleep (* (long secs) 1000))))
+
+(defn sleep-until-secs [target-secs]
+ (Time/sleepUntil (* (long target-secs) 1000)))
+
+(def ^:const sig-kill 9)
+
+(def ^:const sig-term 15)
+
+(defn send-signal-to-process
+ [pid signum]
+ (try-cause
+ (exec-command! (str (if on-windows?
+ (if (== signum sig-kill) "taskkill /f /pid " "taskkill /pid ")
+ (str "kill -" signum " "))
+ pid))
+ (catch ExecuteException e
+ (log-message "Error when trying to kill " pid ". Process is probably already dead."))))
+
+(defn force-kill-process
+ [pid]
+ (send-signal-to-process pid sig-kill))
+
+(defn kill-process-with-sig-term
+ [pid]
+ (send-signal-to-process pid sig-term))
+
+(defn ensure-process-killed!
+ [pid]
;; TODO: should probably do a ps ax of some sort to make sure it was killed
(try-cause
- (exec-command! (str (if on-windows? "taskkill /f /pid " "kill -9 ") pid))
+ (kill-process-with-sig-term pid)
(catch ExecuteException e
(log-message "Error when trying to kill " pid ". Process is probably already dead."))))
+(defn add-shutdown-hook-with-force-kill-in-1-sec
--- End diff --
I was thinking that this would not run some other function before calling sleep. The Shutdown hook runs when the process is going to exit, if the other function gets stuck (which I was able to do in my testing) then the process never exits.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46592535
@revans2 When you have a chance can you please review the latest change? I have implemented what you proposed in your last comment.
---
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] incubator-storm pull request: STORM-183: Replacing RunTime.halt() ...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/incubator-storm/pull/143#issuecomment-46717551
Superviosr cleanup order has been updated.
---
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.
---