Andrew Wong 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 17: Code-Review+1 (11 comments) Overall looks good, just a few more nits. http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/build.gradle File java/kudu-subprocess/build.gradle: http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/build.gradle@46 PS17, Line 46: jar { : manifest { : attributes( : 'Main-Class': 'org.apache.kudu.subprocess.EchoSubprocessMain' : ) : } : } Is it expected that this builds kudu-subprocess.jar and not some echo-subprocess.jar? http://gerrit.cloudera.org:8080/#/c/14329/17/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/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@83 PS17, Line 83: If fails to read nit: "If it fails to read" http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@123 PS17, Line 123: a four bytes nit: "a four-byte array", same below http://gerrit.cloudera.org:8080/#/c/14329/17/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/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageReader.java@88 PS17, Line 88: it the code higher nit: "propagate it up the call stack" http://gerrit.cloudera.org:8080/#/c/14329/17/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/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageWriter.java@43 PS17, Line 43: private static final Logger LOG = LoggerFactory.getLogger(MessageWriter.class); Not sure if you're seeing this, but when I run the subprocess from a C++-managed process, I see this logged first: log4j:WARN No appenders could be found for logger (org.apache.kudu.subprocess.MessageReader). log4j:WARN No appenders could be found for logger (org.apache.kudu.subprocess.MessageWriter). log4j:WARN Please initialize the log4j system properly. log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info. log4j:WARN Please initialize the log4j system properly. log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info. http://gerrit.cloudera.org:8080/#/c/14329/13/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/13/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@42 PS13, Line 42: X_WR nit: add a space http://gerrit.cloudera.org:8080/#/c/14329/17/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/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@33 PS17, Line 33: private static final String QUEUE_SIZE_SHORT_OPTION = "q"; What's the point of defining all these variables up front instead of in-lining them where they're used? This isn't a pattern we really see in our C++ tooling; why here? Same with other files and log error messages. In general, "magic numbers" deserve constants; but these don't seem magical at all, and having constants means jumping back and forth between where they're used and where they're defined to understand things. http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@34 PS17, Line 34: queuesize nit: camelCase for this? http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessConfiguration.java@36 PS17, Line 36: Size of the message queue for subprocess" nit: maybe "Maximum number of messages held by the message queue"? so it's clear the units here are messages and not bytes or something. http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java: http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@95 PS17, Line 95: final Function<Throwable, Object> noErrors = e -> { : LOG.error(String.format("Unexpected error: %s", e.getMessage())); : Assert.assertTrue(false); : return null; : }; nit: maybe declare this to be static as NO_ERRORS and reuse it elsewhere. Same with HAS_ERROR? http://gerrit.cloudera.org:8080/#/c/14329/17/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestEchoSubprocess.java@180 PS17, Line 180: final boolean injectInterrupt = true; nit: why not inline this and annotate with a comment? Same above? -- 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: 17 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Thu, 06 Feb 2020 18:59:09 +0000 Gerrit-HasComments: Yes
