[STORM-3385] Avoid two threads reading from the same input stream of a process #3005
Conversation
|
Won't the thread here be skipped, since exitCallback is null? https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/daemon/supervisor/ClientSupervisorUtils.java#L141 |
|
@srdo Yes it's the purpose of making it to null. So that separate thread can be skipped. Then there will be only one thread reading from process.getInputStream() We also want exitCallback to be null since Line 74 is |
|
No, I mean exitCodeCallback is already null in the line you're changing https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/daemon/supervisor/ClientSupervisorUtils.java#L67. Since it is already null, we can't hit the case where Utils.asyncLoop is called. |
|
No. |
|
Sorry, I obviously wasn't paying enough attention when looking at this. +1 |
|
No worries. Thanks for review. |
| @@ -64,7 +64,7 @@ public static int processLauncherAndWait(Map<String, Object> conf, String user, | |||
| final Map<String, String> environment, final String logPreFix) | |||
| throws IOException { | |||
| int ret = 0; | |||
| Process process = processLauncher(conf, user, null, args, environment, logPreFix, null, null); | |||
| Process process = processLauncher(conf, user, null, args, environment, null, null, null); | |||
There was a problem hiding this comment.
I agree this is a good change.
It isn't obvious to me that adding a prefix should cause the threading behavior. I'd rather see an explicit API with either a name change or a thread parameter rather than relying on logPrefix.
There was a problem hiding this comment.
Yes I totally agree. Filed a separate jira: https://issues.apache.org/jira/browse/STORM-3389
| @@ -64,7 +64,7 @@ public static int processLauncherAndWait(Map<String, Object> conf, String user, | |||
| final Map<String, String> environment, final String logPreFix) | |||
| throws IOException { | |||
| int ret = 0; | |||
| Process process = processLauncher(conf, user, null, args, environment, logPreFix, null, null); | |||
| Process process = processLauncher(conf, user, null, args, environment, null, null, null); | |||
| if (StringUtils.isNotBlank(logPreFix)) { | |||
There was a problem hiding this comment.
Why not remove this here instead for consistency? I just searched all the callers and all except here rely on processLauncher and don't call Utils.readAndLogStream.
There was a problem hiding this comment.
I think the difference is that processLauncherAndWait wait for the process to finish, which is synchronous. processLauncher doesn't wait, which is asynchronous.
There was a problem hiding this comment.
I meant do we really need to handle Utils.readAndLogStream here instead of deferring in processLauncher? If not we can just remove the if statement instead.
There was a problem hiding this comment.
Do we want to create a new thread just for readAndLogStream? I think it makes sense to keep Utils.readAndLogStream here. Btw this is the only caller of processLauncher that set processExitCallback parameter to null.
There was a problem hiding this comment.
OK I got it. You're right. What we should do it is STORM-3389 instead, the change here looks reasonable.
https://issues.apache.org/jira/browse/STORM-3385
This is not causing any problem so far. But having two threads are reading from the same input stream doesn't seem right.