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 2: (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 thread th > Could we just call them reader and writer then? Producer and consumer is a Done http://gerrit.cloudera.org:8080/#/c/14329/1//COMMIT_MSG@15 PS1, Line 15: optional JSON encoding) from > Can you elaborate on why this is useful? The main use case I know of is for I think this is mainly a match up with the C++ side protobuf format support. Other than that, I can only think of better debuggability as message is human readable format. 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: : package org.apache.kudu.subprocess; : : import org.apache.kudu.subprocess.Subprocess.EchoRequestPB; : import org.apache.kudu.subprocess.Subprocess.EchoResponsePB; : : /** : * Class that processes a EchoRequest and simply echoes the request : * as a response. : */ : public class EchoProtocolProcessor implements ProtocolProcessor<EchoRequestPB, EchoResponsePB> { : : @Override : public EchoResponsePB response(EchoRequestPB request) { : EchoResponsePB.Builder resBuilder = EchoResponsePB.newBuilder(); : resBuilder.setDat > Delete Done 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 final Strin > Sorry if this is a newbie java question, but doesn't this mean that this is Yeah, you are right! Sorry, my mistakes. 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 MessageP The given BufferedInputStream (or BufferedOutputStream) can be different each time the read/write method calls (especially for the test cases). And is not necessarily known to MessageProcessor at the initiation time. 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: a.isEmp > Should this be local to the MessageProducer rather than static? Yeah, my mistake. 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: Responds to a protobuf request messa > nit: "Responds to a protobuf request message." Done 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 th I think it is more specific in this way, as there are some configurations that can be local to different subprocess instances. 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 a Noted, although I would prefer to move the stress test to a following commit. 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 EchoRequest I guess so, but what is the intention behind? Moreover, with the current approach we can easily add more generic field (e.g request ID) in SubprocessRequestPB(or SubprocessResponsePB) that are required for all requests/responses, instead of having them in each request/response definition. -- 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: 2 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 14 Oct 2019 16:58:31 +0000 Gerrit-HasComments: Yes
