Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20173 )
Change subject: IMPALA-12268: Fix possible blocking when use java.lang.Process. ...................................................................... Patch Set 3: (3 comments) Thanks for uploading a fix! It makes sense to me. Just have some minor comments. http://gerrit.cloudera.org:8080/#/c/20173/3/fe/src/test/java/org/apache/impala/common/InputStreamGobbler.java File fe/src/test/java/org/apache/impala/common/InputStreamGobbler.java: http://gerrit.cloudera.org:8080/#/c/20173/3/fe/src/test/java/org/apache/impala/common/InputStreamGobbler.java@77 PS3, Line 77: throw new RuntimeException(e); I think this won't be caught. Should we log the error instead? http://gerrit.cloudera.org:8080/#/c/20173/3/fe/src/test/java/org/apache/impala/customcluster/RunShellCommand.java File fe/src/test/java/org/apache/impala/customcluster/RunShellCommand.java: http://gerrit.cloudera.org:8080/#/c/20173/3/fe/src/test/java/org/apache/impala/customcluster/RunShellCommand.java@65 PS3, Line 65: String stderr = stderrBuf.toString(); Should we wait until the stderrGobbler thread finish? http://gerrit.cloudera.org:8080/#/c/20173/3/fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java File fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java: http://gerrit.cloudera.org:8080/#/c/20173/3/fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java@42 PS3, Line 42: InputStreamGobbler stdoutGobbler = Why do we need these? The original code doesn't collect any outputs. -- To view, visit http://gerrit.cloudera.org:8080/20173 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06b728e6134fa14d3970ea7db4d64a962bb2d694 Gerrit-Change-Number: 20173 Gerrit-PatchSet: 3 Gerrit-Owner: Xiang Yang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 11 Jul 2023 02:53:50 +0000 Gerrit-HasComments: Yes
