You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by Lewuathe <gi...@git.apache.org> on 2015/03/04 14:18:45 UTC
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
GitHub user Lewuathe opened a pull request:
https://github.com/apache/storm/pull/456
[STORM-695] storm CLI tool reports zero exit code on error scenario
Handling `java.lang.RuntimeException` and `NotAliveException` in storm command.
This feature can be useful for handling a exit code returned from shell command.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Lewuathe/storm STORM-695
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/456.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 #456
----
commit 7e36ce579f0dcbcf4dfe5942b906a6e94a1815e8
Author: lewuathe <le...@me.com>
Date: 2015-03-04T13:14:51Z
[STORM-695] storm CLI tool reports zero exit code on error scenario
----
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by Lewuathe <gi...@git.apache.org>.
Github user Lewuathe commented on the pull request:
https://github.com/apache/storm/pull/456#issuecomment-83543346
@revans2 @HeartSaVioR Thank you for reviewing.
We should not distinguish error types with exit code. So JVM return code informs us the occurrence of some exception, I'll update to return exit code 1 when any exception has been occurred.
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by miguno <gi...@git.apache.org>.
Github user miguno commented on the pull request:
https://github.com/apache/storm/pull/456#issuecomment-77159580
Many thanks for the PR, Kai!
> However if the topology is submitted with `storm shell`, this cannot track exception because spawnvp seems to return no stdout and stderr.
See [my comment](https://issues.apache.org/jira/browse/STORM-695?focusedCommentId=14340333&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14340333) in the STORM-695 JIRA ticket. `os.spawnvp` returns the exit code of the process:
```python
# return code can be 0 or non-zero, i.e. there will be no exception thrown like for sub.check_output
return_code = os.spawnvp(os.P_WAIT, JAVA_CMD, all_args)
```
However the problem is that Storm is currently not returning any non-zero return codes, even in the face of errors. I haven't had the chance to test-drive your code yet, but the code in the PR might not work for the same reason:
```python
try:
ret = sub.check_output(all_args, stderr=sub.STDOUT)
print(ret)
except sub.CalledProcessError as e:
```
If I understand the Python docs correctly, the `CalledProcessError` will only be thrown if the process returns with a non-zero exit code, and with current Storm this won't happen -- the return code will always be zero I think.
One workaround could be to slightly adjust the code in the PR to simply grep for error messages in `STDOUT` / `STDERR` -- regardless of what the return code of the process is -- and, if there is a match, consider the process/command failed.
(And please correct me if I'm wrong.)
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/456#issuecomment-78397516
IMHO I agree @revans2, parsing stdout by regular expression would become easily broken by later commits.
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by Lewuathe <gi...@git.apache.org>.
Github user Lewuathe commented on a diff in the pull request:
https://github.com/apache/storm/pull/456#discussion_r25820615
--- Diff: bin/storm ---
@@ -183,7 +209,16 @@ def exec_storm_class(klass, jvmtype="-server", jvmopts=[], extrajars=[], args=[]
os.spawnvp(os.P_WAIT, JAVA_CMD, all_args)
else:
# handling whitespaces in JAVA_CMD
- sub.call(all_args)
+ try:
+ ret = sub.check_output(all_args, stderr=sub.STDOUT)
+ print(ret)
--- End diff --
Because `check_output` absolutes all outout from command. It is necessary to `print` alternatively to see stdout.
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by miguno <gi...@git.apache.org>.
Github user miguno commented on a diff in the pull request:
https://github.com/apache/storm/pull/456#discussion_r25774112
--- Diff: bin/storm ---
@@ -183,7 +209,16 @@ def exec_storm_class(klass, jvmtype="-server", jvmopts=[], extrajars=[], args=[]
os.spawnvp(os.P_WAIT, JAVA_CMD, all_args)
else:
# handling whitespaces in JAVA_CMD
- sub.call(all_args)
+ try:
+ ret = sub.check_output(all_args, stderr=sub.STDOUT)
+ print(ret)
--- End diff --
Why the print?
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by miguno <gi...@git.apache.org>.
Github user miguno commented on the pull request:
https://github.com/apache/storm/pull/456#issuecomment-78243873
Sorry that I haven't had the time yet to follow-up. Please stay tuned!
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/456#issuecomment-169130180
@Lewuathe I am sorry that this fell off of my radar. The code looks great. I am +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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by Lewuathe <gi...@git.apache.org>.
Github user Lewuathe commented on the pull request:
https://github.com/apache/storm/pull/456#issuecomment-77156141
However the topology is submitted with `storm shell`, this cannot track exception because `spawnvp` seems to return no stdout and stderr. I don't know the reason why in this case `spawnvp` is called. Can I migrate these two cases into `subprocess.check_output`?
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/456#issuecomment-78392307
Why do we want to distinguish between different errors in the exit code?
I would rather see the commands themselves handle this instead of the python code, the coupling between the script and the java code is too high for me to feel comfortable relying on this functionality. If I have a java program that catches the exception and outputs the error differently this code will not work. What is more the original problem still exists in those cases, and storm will exit with a 0 exit code on error.
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by miguno <gi...@git.apache.org>.
Github user miguno commented on a diff in the pull request:
https://github.com/apache/storm/pull/456#discussion_r25774104
--- Diff: bin/storm ---
@@ -183,7 +209,16 @@ def exec_storm_class(klass, jvmtype="-server", jvmopts=[], extrajars=[], args=[]
os.spawnvp(os.P_WAIT, JAVA_CMD, all_args)
else:
# handling whitespaces in JAVA_CMD
- sub.call(all_args)
+ try:
+ ret = sub.check_output(all_args, stderr=sub.STDOUT)
+ print(ret)
+ except sub.CalledProcessError as e:
+ # Handling exception with stdout and stderr
+ if e.returncode != 0:
--- End diff --
I think this check is redundant, given what the Python docs say:
> subprocess.check_output(): [...] If the return code was non-zero it raises a CalledProcessError.
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/456
---
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] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
Posted by Lewuathe <gi...@git.apache.org>.
Github user Lewuathe commented on the pull request:
https://github.com/apache/storm/pull/456#issuecomment-77264323
@miguno Thank you for comment. I tested the error handling and exit code of current storm command. And I found when any exception has occurred `check_output` returns non-zero code(just only 1). So with this patch, storm command can detect the occurrence of exception. But error exit code always be 1, so in order to specify what exception is occurred, we must parse stdout and stderr. This is what I was trying to do with 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.
---