Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14329 )
Change subject: [java] KUDU-2971: process communicates via protobuf-based protocol ...................................................................... Patch Set 16: (9 comments) http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java: http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@104 PS15, Line 104: ith the underlying buffer size being the : * maximum bytes of a message, the write is atomic. That i > Does it mean the buffer size is always greater than the buffer in > BufferedOutputStream I guess you mean the buffer size of the BufferedOutputStream is always greater than (or at least equal to) the message size. Yes, it is the case as documented at https://gerrit.cloudera.org/c/14329/15/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java#92 (the BufferedOutputStream is initiated with the maximum bytes of a message to ensure the underlying buffer can hold the entire message before flushing). Sorry I should be clear here as well, I will update the comment. http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessCommandLineParser.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessCommandLineParser.java: http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessCommandLineParser.java@91 PS14, Line 91: : : : > Oh, I just meant that: Ack http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java: http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@51 PS15, Line 51: private static Function<Throwable, Object> ERROR_HANDLER = (t) -> { > Could be final? As we have constructor assign errorHandler again(SubprocessExecutor(Function<Throwable, Object> errorHandler)), it is not possible to be final. http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@59 PS15, Line 59: SubprocessExecutor() { : } > Can be removed? This is needed for non-test case in SubprocessExecutor. http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java: http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@81 PS15, Line 81: subprocessExecutor.run(args, protocolProcessor, /* timeoutMs= */1000); : } > Nit: move this to just after the declaration of LOG so that it's easier to Done http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@104 PS15, Line 104: /** > I think you can simplify this code by adding (expected = TimeoutException.c Done http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java: http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@41 PS15, Line 41: > reading and writing Done http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@43 PS15, Line 43: > Same comments about using an ExpectedException Rule and a RuleChain. Done http://gerrit.cloudera.org:8080/#/c/14329/15/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageIO.java@53 PS15, Line 53: : public static class PrintStreamOverload extends PrintStream { : public PrintStreamOverload(OutputStream out) { : > Should add a comment explaining what's happening here: you're expanding the Done -- To view, visit http://gerrit.cloudera.org:8080/14329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf9ad24dbc9acc681284b6433836271b5b4c7982 Gerrit-Change-Number: 14329 Gerrit-PatchSet: 16 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 05 Feb 2020 22:13:39 +0000 Gerrit-HasComments: Yes
