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 3: (11 comments) 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: optional JSON encoding) from > Agreed with Andrew and Grant. The "subprocess protocol" module must support Alright, removed the JSON support based on all of your suggestion. 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? Done 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: public class EchoProtocolProcessor implements ProtocolProcessor<EchoRequestPB, EchoResponsePB> { > Should we added Private annotations to all of these classes to make sure no Done 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: for(; retries.intValue() > 0; retries.getAndDecrement()) { > Maybe use a while loop? A for loop without an index initialization is fair Done 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: continue; > Is this an exceptional occurrence? When would the message be empty? Yeah, this is an abnormal case and not expected to happen. Adding the handling in case a correct message is received after a corrupted one. http://gerrit.cloudera.org:8080/#/c/14329/1/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/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessCommandLineParser.java@38 PS1, Line 38: public class SubprocessCommandLineParser { > But can't you pass in arguments to a Java program as system properties? E.g There could be a bunch of properties (that can change), like the size of the blocking queue, which would be easier to manage with config file. http://gerrit.cloudera.org:8080/#/c/14329/1/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/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMain.java@55 PS1, Line 55: static void run(SubprocessConfiguration conf) { > Could be private? Or if this visibility is only for testing, annotate with Done 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? This is a randomly chosen number, but I make it configurable to be able customize it. 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`. Done 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. Done http://gerrit.cloudera.org:8080/#/c/14329/1/src/kudu/subprocess/subprocess.proto File src/kudu/subprocess/subprocess.proto: http://gerrit.cloudera.org:8080/#/c/14329/1/src/kudu/subprocess/subprocess.proto@24 PS1, Line 24: message EchoRequestPB { : required string data = 1; : } : message EchoResponsePB { : required string data = 1; : } > Ah I see this is based off of the work in tools/tool.proto. I moved to the protobuf definition to https://gerrit.cloudera.org/c/14426/ as it is required for both Java and C++ patches. I adopted Adar's suggestion to use Protobuf Any support and your suggestion to templatize the message type. -- 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: 3 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, 18 Nov 2019 18:10:24 +0000 Gerrit-HasComments: Yes
