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 1: (8 comments) I did a quick first pass. I think the overall approach looks good. http://gerrit.cloudera.org:8080/#/c/14329/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14329/1//COMMIT_MSG@15 PS1, Line 15: (with optional JSON encoding) > Looked at your other patch, and I now get what you mean that you want to ke I agree I don't see a strong need for it. We could remove all the configuration code and json code without it. It also simplifies the test matrix. http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/build.gradle File java/kudu-subprocess/build.gradle: http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/build.gradle@26 PS2, Line 26: compile libs.slf4jsimple What is slf4jsimple required for? http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolProcessor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolProcessor.java: http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolProcessor.java@27 PS2, Line 27: // Unless required by applicable law or agreed to in writing, Should we added Private annotations to all of these classes to make sure no ones tries to use them? http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java: http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java@95 PS2, Line 95: break; Maybe use a while loop? A for loop without an index initialization is fairly uncommon. http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProducer.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProducer.java: http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProducer.java@96 PS2, Line 96: try { Is this an exceptional occurrence? When would the message be empty? http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java: http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java@41 PS2, Line 41: private static final int QUEUE_SIZE = 100; How was this size chosen? http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessRuntimeException.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessRuntimeException.java: http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessRuntimeException.java@23 PS2, Line 23: final class SubprocessRuntimeException extends RuntimeException { Maybe make this clearly Kudu related. `KuduSubprocessException`. http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/resources/log4j.properties File java/kudu-subprocess/src/main/resources/log4j.properties: http://gerrit.cloudera.org:8080/#/c/14329/2/java/kudu-subprocess/src/main/resources/log4j.properties@1 PS2, Line 1: log4j.debug=true Match the log4j2.properties files like in all other modules. -- 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: 1 Gerrit-Owner: Hao Hao <[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, 21 Oct 2019 16:03:06 +0000 Gerrit-HasComments: Yes
