[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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=14340333page=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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[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 lewua...@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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-695] storm CLI tool reports zero exit c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
