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: (1 comment) 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; : } > message EchoRequestPB { Ah I see this is based off of the work in tools/tool.proto. Adar had this to say there: // Because the control shell communicates via pipe and not krpc, we can't make // use of service dispatch and must instead multiplex all command requests and // responses via ControlShellRequestPB and ControlShellResponsePB respectively. This is saying that the reason it is written with oneof instead of the header approach is that the cluster CLI needs to be able to perform any of the Create/Delete/Start/etcCluster commands. That isn't the case here -- I'd guess each Java subprocess only cares about processing one kind of request, similar to krpc. If so, I would be more in favor of using headers instead of multiplexing. I left feedback on the other patch that maybe we should reuse less of the cluster CLI. -- 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:55:32 +0000 Gerrit-HasComments: Yes
