Hao Hao 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 6: (11 comments) http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/build.gradle File java/kudu-subprocess/build.gradle: http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/build.gradle@25 PS6, Line 25: compile libs.hadoopCommon > Is this included just for Configuration.Java? There should be something mor Hmm, it seems I cannot find another more lightweight to include than hadoopCommon. Which one you are talking about http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/build.gradle@56 PS6, Line 56: configurations.compile.collect { it.isDirectory() ? it : zipTree(it) } > The shadow plugin, included above, should handle the shading of this jar fo Done http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/BasicSubprocessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/BasicSubprocessor.java: http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/BasicSubprocessor.java@45 PS6, Line 45: BasicSubprocessor > nit: there's not other subprocessor type, so maybe just call this Subproces Done http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/BasicSubprocessor.java@96 PS6, Line 96: catch (IOException e) { : LOG.error("Unable to close the underlying output stream", e); : } > Will this ever catch anything if we're doing System.exit(1) when we hit an Are you asking if close() will be called with System.exit(1)? The oracle Java doc said "The try-with-resources statement ensures that each resource is closed at the end of the statement". However, the statement is not very clear about System.exit() situation. So I did an experiment to extend BufferedInputStream with customized close() to verify if close() is called and if the catch block get executed when close() throw IOException. The answer is 'Yes' for both. 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; : } : : MessageProcessor(long maxMessageBytes, : BufferedInputStream in) { : Preconditions.checkNotNull(in); : this.maxMessageBytes = maxMessageBytes; : this.in = in; : } : : MessageProcessor(long maxMessageBytes, : BufferedOutputStream out) { : Preconditions.checkNotNull(out); : this.maxMessageBytes = maxMessageBytes; : this.out = out; : } > nit: Why do we need so many constructors if we have MessageProcessor(long m Done http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@52 PS6, Line 52: this.maxMessageBytes = maxMessageBytes; : this.in = in; : } : : MessageProcessor(long maxMessageBytes, : BufferedOutputStream out) { : Preconditions.checkNotNull(out); : this.maxMessageBytes = maxMessageBytes; : this.out = out; : } : : MessageProcessor(long maxMessageBytes, : BufferedInputStream in, : BufferedOutputStream out) { : Preconditions.checkNotNull(in); : Preconditions.checkNotNull(out); : this.maxMessageBytes = maxMessageBytes; : this.in = in; : this.out = out; > nit: these could use setInputStream() and setOutputStream() Done http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@92 PS6, Line 92: * @throws IllegalStateException if the message is malformed > No longer true? This is refer to L106 and L113 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; > If we are interrupted, but we try and eventually succeed at putting the mes Are you asking should we do that? I don't quite understand why we still want to break out the larger loop in this case? Why not proceed to the next message? http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@104 PS6, Line 104: Restore > nit: "Return"? "Propagate"? Done http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/6/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java@41 PS6, Line 41: IllegalArgumentException > InvalidProtocolBufferException? Done 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); > Requiring a configuration file means that Kudu deployments that want to use I feel configuration file is more flexible if we have more (or less) configs in future. And we can programmatically generate the config file as we do for sentry and hms (Similar for Ranger configurations). But I can update that if you think strongly otherwise. -- 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: 6 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: Tue, 17 Dec 2019 03:19:20 +0000 Gerrit-HasComments: Yes
