Andrew Wong 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: (9 comments) Didn't look over the tests yet, but here are a couple high-level suggestions and some nits. 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 Subprocessor 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 exception? 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 maxMessageBytes), setInputStream(), and setOutputStream()? 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() 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? 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 message on the queue, will we still break out of the larger loop and interrupt the caller? 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"? 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? 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 this will need to create a configuration file and somehow pass that into Kudu. Given that the arguments we're passing here are relatively simple, how do you feel about passing in num writers, queue size, and max bytes as arguments instead? Eventually I think we'll need config files (e.g. for Ranger configurations), but I don't think the subprocess arguments need to abide the same file-based configuration -- we'd just pull in ranger-site.xml or something. -- 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: Mon, 16 Dec 2019 21:30:08 +0000 Gerrit-HasComments: Yes
