Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14329 )
Change subject: [java] process communicates via protobuf-based protocol ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/14329/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14329/1//COMMIT_MSG@14 PS1, Line 14: reader(producer) Could we just call them reader and writer then? Producer and consumer is a little unintuitive IMO because when I'm writing, I'm producing, and when I'm reading, I'm consuming. http://gerrit.cloudera.org:8080/#/c/14329/1//COMMIT_MSG@15 PS1, Line 15: (with optional JSON encoding) Can you elaborate on why this is useful? The main use case I know of is for structured, schema-ed messages, which makes protobuf a natural fit. Why bother with all the extra code for JSON? http://gerrit.cloudera.org:8080/#/c/14329/1/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/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/EchoProtocolProcessor.java@17 PS1, Line 17: // Licensed to the Apache Software Foundation (ASF) under one : // or more contributor license agreements. See the NOTICE file : // distributed with this work for additional information : // regarding copyright ownership. The ASF licenses this file : // to you under the Apache License, Version 2.0 (the : // "License"); you may not use this file except in compliance : // with the License. You may obtain a copy of the License at : // : // http://www.apache.org/licenses/LICENSE-2.0 : // : // Unless required by applicable law or agreed to in writing, : // software distributed under the License is distributed on an : // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY : // KIND, either express or implied. See the License for the : // specific language governing permissions and limitations : // under the License. Delete http://gerrit.cloudera.org:8080/#/c/14329/1/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/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageConsumer.java@42 PS1, Line 42: static int retries Sorry if this is a newbie java question, but doesn't this mean that this is shared among all instantiations of MessageConsumer? http://gerrit.cloudera.org:8080/#/c/14329/1/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/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProcessor.java@160 PS1, Line 160: BufferedInputStream in Why bother passing these around? Why not just make them members of MessageProcessor? http://gerrit.cloudera.org:8080/#/c/14329/1/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/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageProducer.java@95 PS1, Line 95: retries Should this be local to the MessageProducer rather than static? http://gerrit.cloudera.org:8080/#/c/14329/1/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/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolProcessor.java@31 PS1, Line 31: Responses a protobuf request message nit: "Responds to a protobuf request message." 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 { Maybe a newbie java question, but why go through the trouble of building this out as a tool with CLI arguments and a configuration file, versus running a jar and using system properties? http://gerrit.cloudera.org:8080/#/c/14329/1/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProducerConsumer.java File java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProducerConsumer.java: http://gerrit.cloudera.org:8080/#/c/14329/1/java/kudu-subprocess/src/test/java/org/apache/kudu/subprocess/TestMessageProducerConsumer.java@131 PS1, Line 131: MessageConsumer consumer = new MessageConsumer(blockingQueue, messageProcessor, out); : for (int i = 0; i < CONSUMER_TASK_NUM; i++) { : CompletableFuture consumerFuture = : CompletableFuture.runAsync(consumer, consumerService); : consumerFuture.exceptionally(errorHandler); : } It'd be nice if we had a test that stressed this with many input messages and a bit of slowness in the processing to see what happens if the consumers try to write in parallel. 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; : } Are java generics not expressive enough to allow us to just use EchoRequestPB/EchoResponsePB directly (if we put the AppStatusPB error into these messages), rather than wrapping with a SubprocessRequestPB? -- 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: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 01 Oct 2019 06:45:38 +0000 Gerrit-HasComments: Yes
