[GitHub] storm pull request: [STORM-442] multilang ShellBolt/ShellSpout die...
Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/305 --- 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-442] multilang ShellBolt/ShellSpout die...
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/305#issuecomment-68936183 No other comments in the past two weeks so I am going to check this 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm pull request: [STORM-442] multilang ShellBolt/ShellSpout die...
Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/305#issuecomment-67989780 I see this has not had that much traffic in quite a while. The code to me looks acceptable. I am not really happy with calling .available() twice, but I can live with it. There is the possibility of a race if two different threads call getErrorsString or logErrorStream at the same time, but as the way the code is currently used that should not be a problem. I am +1 on checking this in, but would like to know if others have any option. --- 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-442] multilang ShellBolt/ShellSpout die...
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/305#issuecomment-61774651 @clockfly I think it is not a good idea to kill sub process. Because when we catch a exception, we do not know whether sub process is still running. kill pid may cause other error. By the way, when parent process is exist, the sub process will exit when read from stdin encounter a EOF, so sub process will not zoombie. --- 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-442] multilang ShellBolt/ShellSpout die...
Github user itaifrenkel commented on the pull request: https://github.com/apache/storm/pull/305#issuecomment-61631559 @clockfly - Please clarify. When you say the parent process fails do you mean that the worker process is no longer running? If this is the case that process cannot call cleanup() since it is not running. We are using os specific hooks in the child process to receive signal when parent process dies. Another cross platform alternative is using file lock checks. Either way, are you sure this is related to this pull request ? --- 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-442] multilang ShellBolt/ShellSpout die...
Github user clockfly commented on the pull request: https://github.com/apache/storm/pull/305#issuecomment-61631943 @itaifrenkel, ``` When you say the parent process fails do you mean that the worker process is no longer running? ``` No, it means the case described by dashengju, that an exception is thrown in the java space(not originated from sub process) --- 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-442] multilang ShellBolt/ShellSpout die...
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/305#discussion_r19839876
--- Diff: storm-core/src/jvm/backtype/storm/utils/ShellProcess.java ---
@@ -135,7 +135,14 @@ public void logErrorStream() {
public String getErrorsString() {
if (processErrorStream != null) {
try {
-return IOUtils.toString(processErrorStream);
+StringBuilder sb = new StringBuilder();
+while (processErrorStream.available() 0) {
+int bufferSize = processErrorStream.available();
--- End diff --
I'm curious to. Is it not safe because of chance of blocking, or
available() can always return 0 with some InputStream subclasses so we cannot
read any messages from subprocess?
---
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-442] multilang ShellBolt/ShellSpout die...
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/305#issuecomment-61447792 @HeartSaVioR yes, there is a chance to occur that subprocess writes stderr for some reason but parent process runs well without Exception. When this happens, parent process will check, and log then to log system. Below is the ShellBolt and PythonBolt communication flow. As you can see, when the ShellBolt send a tuple to Python, it will read from stderr(call logErrorStream()) to see anything to log, and log info them.  Below is the ShellSpout and PythonSpout communication flow. When the ShellSpout ask for a new tuple, it will read from stderr(call logErrorStream()) to see anything to log, and log info them.  --- 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-442] multilang ShellBolt/ShellSpout die...
Github user HeartSaVioR commented on the pull request: https://github.com/apache/storm/pull/305#issuecomment-61269957 If we're narrowing issue to just handle parent process's Exception hang, looks good to me. (We can make it better with extracting logic that reads from stderr to method, since it's duplicated from logErrorStream() and getErrorsString(). ) This could be off-topic, but is there no chance to occur that subprocess writes stderr for some reason but parent process runs well without Exception? If it can possible, is parent process responsible to read stderr and do something (like logging, raise Exception, etc.)? --- 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-442] multilang ShellBolt/ShellSpout die...
Github user dashengju commented on the pull request: https://github.com/apache/storm/pull/218#issuecomment-61038451 @HeartSaVioR , parent process's exception contains two types: 1) net io error caused; for example, subprocess report an error to stderr and exit; 2) parent process's handle logic exception. as 1), we want to read from stderr to get more information; as 2), as you say, it may not be right way to capture it when only parent process raises exception. @d2r 's suggestion, we should change ShellProcess#getErrorsStream so that it checks that there is actually something to read. --- 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. ---
