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 7: (4 comments) 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@96 PS6, Line 96: : : > Are you asking if close() will be called with System.exit(1)? The oracle Ja Ah, I was asking how this interacts, if at all, with ERROR_HANDLER, but based on your explanation, it seems this is mostly for catching errors with 'in' and 'out'. Also, are we guaranteed that the error came from the output stream? Could it have come from the input stream? 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@92 PS6, Line 92: } > This is refer to L106 and L113 Ah, didn't realize Preconditions meant we didn't have to annotate with 'throws IllegalStateException'. Thanks for explaining. 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; > Are you asking should we do that? I don't quite understand why we still wan I'm asking whether that's what we're doing. 'finally' blocks are run unconditionally, so if we _were_ interrupted, but we retried and eventually succeeded, won't this still interrupt the caller? Is that desired? 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 feel configuration file is more flexible if we have more (or less) config I'm imagining how this would work end-to-end and using config files, I think, means either: - That the caller of this tool (ie. the Kudu master) needs to generate a config file for each subprocess, based on gflags that are passed to the master. Or, - Cloudera Manager (or equivalent) would need to generate a config file based on some configuration through CM, for each subprocess service. Yes this is _similar_ to what we do for Sentry and the HMS, but it's different in that this would have to be done for every subprocess, meaning we would have a config file for Ranger, a config file for Atlas, a config file for the Kudu-Ranger subprocess, _and_ a config file for the Kudu-Atlas subprocess -- it just seems like a lot. Contrast that to using command-line arguments for the binary -- the caller (ie the Kudu master) would run the tool, passing arguments directly to the binary, and we'd only need the Ranger and Atlas config files. I think these are all equally flexible, but having command-line arguments seems like the path of least resistance here, and the least unwieldy IMO. -- 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: Tue, 17 Dec 2019 04:10:16 +0000 Gerrit-HasComments: Yes
