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 15: (17 comments) > Patch Set 14: > > (17 comments) > > Overall looks much cleaner and simpler than the last revision I looked at. > Thanks for making all of the changes. > > Note: I didn't review the test code. Thanks a lot for the review! http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolHandler.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolHandler.java: http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolHandler.java@34 PS14, Line 34: respBuilde > Nit: elsewhere in the C++ code we generally use 'resp' as the prefix for th Done http://gerrit.cloudera.org:8080/#/c/14329/14/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/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@34 PS14, Line 34: * Util class for reading and writing protobuf messages. > Nit: reading and writing protobuf messages. Done http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@98 PS14, Line 98: expect > expected Done http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@105 PS14, Line 105: will b > will Done http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java: http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@79 PS14, Line 79: exit > Nit: exiting Done http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@90 PS14, Line 90: if (data.length == 0) { > Under what circumstances would we expect to see an empty message? Doc? We don't expect to see empty messages in general. Updated the comment. http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java: http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@37 PS14, Line 37: o > one Done http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@65 PS14, Line 65: while (true) { > Why do we have to test this condition? Why not just while (true)? If we're Hmm, you're right, missed this one, thanks! http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@92 PS14, Line 92: * Constructs a message with the given error status. > Nit: resp Done http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@123 PS14, Line 123: new String(data, StandardCharsets.UTF_8)), e); > This method is simple enough that I think you can inline it into run(). Done 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: : : : > What do you think of passing the entire CommandLine into the SubprocessConf If I understand it correctly, you are suggesting to combine CommandLine with SubprocessConfiguration, so that the caller only need to care about SubprocessConfiguration? If so, looks good to me, updated accordingly. http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java: http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@42 PS14, Line 42: WRITER_ > Nit: add a space before Done http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@44 PS14, Line 44: "Maximum number of threads in the writer thread pool for subprocess"; > Nit: maybe you can shorten the Javadoc for the below getters: Done http://gerrit.cloudera.org:8080/#/c/14329/14/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/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@52 PS14, Line 52: // Exit the program with a nonzero status code if unexpected exception(s) > This isn't actually "visible" for testing. The accessor is (and you've anno Done http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@56 PS14, Line 56: }; : private boolean injectInterrupt = false; : : SubprocessExecutor() { : } : > Can this be defined in the declaration of errorHandler itself? Could it be Done http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@108 PS14, Line 108: for (int i = 0; i < maxWriterThread; i++) { > Can you put the readerFuture into this array too? Then you can simplify the The problem is certain tests depends on the failure of the reader task. If adding readerFuture along with writerFutures, we will get TimeoutException instead of ExecutionException as we are waiting for all tasks to finish. http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java: http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessOutputStream.java@44 PS14, Line 44: @Override > Do we need to provide overrides for the other two write() variants, or for In OutputStream, the other two variants will call into write(int b). But to be safe, I will update to add all. -- 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: 15 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 05 Feb 2020 02:17:51 +0000 Gerrit-HasComments: Yes
