Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14329 )
Change subject: [java] KUDU-2791: process communicates via protobuf-based protocol ...................................................................... Patch Set 7: (6 comments) I did a partial review last night and will post my comments. Some may overlap with reviews since. http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@44 PS6, Line 44: : MessageProcessor(long maxMessageBytes) { : this.maxMessageBytes = maxMessageBytes; : } : : void setInputStream(BufferedInputStream in) { : Preconditions.checkNotNull(in); : this.in = in; : } : : void setOutputStream(BufferedOutputStream out) { : Preconditions.checkNotNull(out); : this.out = out; : } : : /** : * Read a protobuf message, if any, from the underlying buffered input : > Done I just requested constructors on the latest patch. I think one constructor is all that is needed right? Just the one that take all the fields? Sure a couple MessageTest test only need one, but the other can just be null then. http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@92 PS6, Line 92: } > Ah, didn't realize Preconditions meant we didn't have to annotate with 'thr IllegalStateException is a RuntimeException which is an "Unchecked" exception in Java and therefore isn't checked at compile-time and doesn't need to be exposed via the throws keyword. All Unchecked exceptions are direct sub classes of RuntimeException class. http://gerrit.cloudera.org:8080/#/c/14329/7/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/7/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@49 PS7, Line 49: void setInputStream(BufferedInputStream in) { Can this be in the constructor instead? Mutable state like this can get tricky. Same with the output stream below. http://gerrit.cloudera.org:8080/#/c/14329/6/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/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@98 PS6, Line 98: interrupted = true; > I'm asking whether that's what we're doing. 'finally' blocks are run uncond Is there a test case around this retry/finally handling that verifies/show the expected behavior? http://gerrit.cloudera.org:8080/#/c/14329/6/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/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessCommandLineParser.java@69 PS6, Line 69: String confFilePath = cmd.getOptionValue(CONF_LONG_OPTION); > I'm imagining how this would work end-to-end and using config files, I thin I think I agree with Andrew that we could start with command line arguments and grow into full on config files if needed. http://gerrit.cloudera.org:8080/#/c/14329/7/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/7/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@35 PS7, Line 35: private static final int MAX_WRITER_THREAD_DEFAULT = 20; This number feels really high to me, especially as a default. Is there any background to this choice? -- 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: 7 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[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, 18 Dec 2019 19:49:41 +0000 Gerrit-HasComments: Yes
