Adar Dembo 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 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. 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: resBuilder Nit: elsewhere in the C++ code we generally use 'resp' as the prefix for the response rather than 'res'. 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 read and write protobuf message. Nit: reading and writing protobuf messages. 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 http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@105 PS14, Line 105: should will 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 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? 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: a one 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 (!Thread.currentThread().isInterrupted()) { Why do we have to test this condition? Why not just while (true)? If we're interrupted, won't it manifest as a thrown InterruptedException in the call to take()? http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@92 PS14, Line 92: SubprocessResponsePB.Builder res) { Nit: resp http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@123 PS14, Line 123: * Writes the response to the underlying output stream. IOException is fatal, This method is simple enough that I think you can inline it into run(). 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: String queueSize = cmd.getOptionValue(QUEUE_SIZE_LONG_OPTION); : String maxWriterThreads = cmd.getOptionValue(MAX_WRITER_THREADS_LONG_OPTION); : String maxMsgBytes = cmd.getOptionValue(MAX_MESSAGE_BYTES_LONG_OPTION); : conf = new SubprocessConfiguration(queueSize, maxWriterThreads, maxMsgBytes); What do you think of passing the entire CommandLine into the SubprocessConfiguration constructor, and letting it pick out the specific option values it needs? 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: Integer Nit: add a space before http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@44 PS14, Line 44: Nit: maybe you can shorten the Javadoc for the below getters: /** * @return the blah blah blah, or the default value if not provided */ 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: @VisibleForTesting This isn't actually "visible" for testing. The accessor is (and you've annotated it appropriately there), but the field itself is private as expected. http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@56 PS14, Line 56: // Exit the program with a nonzero status code if unexpected exception(s) : // thrown by the reader or writer tasks. : errorHandler = (t) -> { : System.exit(1); : return null; : }; Can this be defined in the declaration of errorHandler itself? Could it be made static too? http://gerrit.cloudera.org:8080/#/c/14329/14/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@108 PS14, Line 108: CompletableFuture[] writerFutures = new CompletableFuture[maxWriterThread]; Can you put the readerFuture into this array too? Then you can simplify the joining code below. 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 close()? -- 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: 14 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: Tue, 04 Feb 2020 22:02:22 +0000 Gerrit-HasComments: Yes
