You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by xiaokang <gi...@git.apache.org> on 2014/03/11 17:19:50 UTC
[GitHub] incubator-storm pull request: STORM-87 fail fast on ShellBolt exce...
GitHub user xiaokang opened a pull request:
https://github.com/apache/incubator-storm/pull/46
STORM-87 fail fast on ShellBolt exception
for jira issue https://issues.apache.org/jira/browse/STORM-87.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/xiaokang/incubator-storm STORM-87
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-storm/pull/46.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 #46
----
commit b1ccc153d924a971d2f1303023b969f8c975b238
Author: Kang Xiao <kx...@gmail.com>
Date: 2014-03-11T16:17:58Z
STORM-87 fail fast on ShellBolt exception
----
---
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-87 fail fast on ShellBolt exce...
Posted by xiaokang <gi...@git.apache.org>.
Github user xiaokang commented on the pull request:
https://github.com/apache/incubator-storm/pull/46#issuecomment-49514453
update code according to @d2r and @revans2 's 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-87 fail fast on ShellBolt exce...
Posted by xiaokang <gi...@git.apache.org>.
Github user xiaokang commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/46#discussion_r15144461
--- Diff: storm-core/src/jvm/backtype/storm/task/ShellBolt.java ---
@@ -241,5 +241,8 @@ private void handleEmit(Map action) throws InterruptedException {
private void die(Throwable exception) {
_exception = exception;
+ LOG.info("Halting process: ShellBolt died.", exception);
+ _collector.reportError(exception);
+ Runtime.getRuntime().halt(11);
--- End diff --
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-87 fail fast on ShellBolt exce...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/46#issuecomment-49626043
I agree a log-error would be more appropriate. Do you want to do that as part of this pull or as a separate one? Either way I am +1 for this change
---
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-87 fail fast on ShellBolt exce...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/46#issuecomment-51496090
+1 it looks good to me. I know @d2r is out for a few days and the changes are so small that I feel fine merging it in now.
---
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-87 fail fast on ShellBolt exce...
Posted by xiaokang <gi...@git.apache.org>.
Github user xiaokang commented on the pull request:
https://github.com/apache/incubator-storm/pull/46#issuecomment-51494523
@dashengju @d2r , The PR is up-merged. @revans2 , I'v replace log-message with log-error in exit-process! function.
---
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-87 fail fast on ShellBolt exce...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/46#discussion_r13729123
--- Diff: storm-core/src/jvm/backtype/storm/task/ShellBolt.java ---
@@ -241,5 +241,8 @@ private void handleEmit(Map action) throws InterruptedException {
private void die(Throwable exception) {
_exception = exception;
+ LOG.info("Halting process: ShellBolt died.", exception);
--- End diff --
Can we make this a `LOG.error` at least?
---
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-87 fail fast on ShellBolt exce...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/incubator-storm/pull/46#issuecomment-50493982
It looks like this needs an up-merge now too.
---
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-87 fail fast on ShellBolt exce...
Posted by xiaokang <gi...@git.apache.org>.
Github user xiaokang commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/46#discussion_r15144460
--- Diff: storm-core/src/jvm/backtype/storm/task/ShellBolt.java ---
@@ -241,5 +241,8 @@ private void handleEmit(Map action) throws InterruptedException {
private void die(Throwable exception) {
_exception = exception;
+ LOG.info("Halting process: ShellBolt died.", exception);
--- End diff --
good idea
---
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-87 fail fast on ShellBolt exce...
Posted by dashengju <gi...@git.apache.org>.
Github user dashengju commented on the pull request:
https://github.com/apache/incubator-storm/pull/46#issuecomment-51449711
@xiaokang
I found a problem with the current die() function describe in https://github.com/apache/incubator-storm/pull/218 , and I think your PR will resolve the problem, but this PR is based old code and you did not modify in ShellSpout, so I copy your code to the new PR.
if you change the code in this PR to solve STORM-442, I will close my 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] incubator-storm pull request: STORM-87 fail fast on ShellBolt exce...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-storm/pull/46
---
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-87 fail fast on ShellBolt exce...
Posted by xiaokang <gi...@git.apache.org>.
Github user xiaokang commented on the pull request:
https://github.com/apache/incubator-storm/pull/46#issuecomment-49514602
While checking the new exit-process! function, I find that it just use log-message to generate an INFO level log. Is it more suitable to use log-error? Since it should be some serious error to exit jvm.
https://github.com/apache/incubator-storm/blob/master/storm-core/src/clj/backtype/storm/util.clj#L317
(defn exit-process!
[val & msg]
(log-message "Halting process: " msg)
(.exit (Runtime/getRuntime) val))
---
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-87 fail fast on ShellBolt exce...
Posted by xiaokang <gi...@git.apache.org>.
Github user xiaokang commented on the pull request:
https://github.com/apache/incubator-storm/pull/46#issuecomment-49815234
OK, I'll update this pull to include the log-error change.
---
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-87 fail fast on ShellBolt exce...
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/46#discussion_r14420063
--- Diff: storm-core/src/jvm/backtype/storm/task/ShellBolt.java ---
@@ -241,5 +241,8 @@ private void handleEmit(Map action) throws InterruptedException {
private void die(Throwable exception) {
_exception = exception;
+ LOG.info("Halting process: ShellBolt died.", exception);
+ _collector.reportError(exception);
+ Runtime.getRuntime().halt(11);
--- End diff --
Now that we have ways of cleanly handling System.exit, can we switch over to that instead of Runtime.halt?
---
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.
---