Andrew Wong 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: (3 comments) Haven't reviewed yet, just responding to 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 > I think this is mainly a match up with the C++ side protobuf format support Why would we want to send JSON over the channel though? If it's for better debugging, why not implement pretty printers on the Java side (e.g. https://github.com/apache/kudu/blob/0f2946b74ea138b29b00f68047c50f44674ddc5a/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java)? And C++ has pb_util that support printing JSON already. I really don't see a really compelling reason keep JSON as a serialization format, and it's a large part of this patch. 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 { > I think it is more specific in this way, as there are some configurations t But can't you pass in arguments to a Java program as system properties? E.g. https://stackoverflow.com/questions/5045608/proper-usage-of-java-d-command-line-parameters Don't those properties only get used by that single Java process? 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; : } > I guess so, but what is the intention behind? Moreover, with the current ap message EchoRequestPB { optional int64 id = 1; optional string data = 2; } message EchoResponsePB { optional int64 id = 1; optional AppStatusPB error = 2; optional string data = 3; } This seems cleaner than having to add a 1) new message and updating 2) SubprocessRequestPB and 3) SubprocessResponsePB. It seems more aligned with the existing code style too, where the underlying protobuf message is the first-class citizen, not the communication channel. For instance, if you look at rpc/rpc_header.proto, we do basically what I'm suggesting with the definition of RequestHeader and ResponseHeader. Also I envision this feature being the addition of binaries along the lines of: java -Dfoo="bar" kuduRangerServer.jar where the Message* classes here are templatized to process specific message types, and the kuduRangerServer has a MessageServer<RangerRequestPB, RangerResponsePB> that encapsulates all the Message* work and doesn't have any switch statements on the message type (because we already know we're handling Ranger requests). I'm not sure what the benefit is of allowing the Message* classes to parse multiple kinds of messages with this oneof switching. -- 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 <[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: Tue, 15 Oct 2019 01:38:42 +0000 Gerrit-HasComments: Yes
