Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14425 )
Change subject: KUDU-2971 p1: add subprocess module ...................................................................... Patch Set 2: Code-Review+1 (1 comment) I think this refactoring makes sense. I'm also not opposed to retaining the JSON support, since it's still being used by "kudu test mini_cluster". I haven't looked at the other patches yet, but I do wonder how we're going to multiplex on top of the subprocess protocol. Thinking out loud: Naively, we could put a lock around the entire subprocess protocol and force users to effectively do: 1. Lock 2. Send my message 3. Wait for and receive response. 4. Unlock But that'll perform poorly as there could only be one outstanding request this way. A better approach would be to allow multiple producers. On the send side, maybe that's done by locking SendMessage and forcing threads to call SendMessage directly. Or maybe it's done via threads posting messages to a blocking queue and a dedicated thread waiting on that queue and calling SendMessage. Would be interesting to compare the two. With multiple producers, the receive side will probably need a dedicated thread blocked on ReceiveMessage. Whenever it gets a new message, it has to somehow match it up with a sender. Using a sequence ID field in the message makes sense, but that muddies the abstraction in that now some aspect of the protobuf definition includes information that's necessary for the transport protocol (rather than the "application"). Anyway, the refactor looks good; the rest of these issues will shake out separately. http://gerrit.cloudera.org:8080/#/c/14425/2/src/kudu/subprocess/subprocess_protocol.cc File src/kudu/subprocess/subprocess_protocol.cc: http://gerrit.cloudera.org:8080/#/c/14425/2/src/kudu/subprocess/subprocess_protocol.cc@45 PS2, Line 45: const int SubprocessProtocol::kMaxMessageBytes = 1024 * 1024; This cap will probably need to be revisited. -- To view, visit http://gerrit.cloudera.org:8080/14425 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If73e27772e1897a04f04229c4906a24c61e361f2 Gerrit-Change-Number: 14425 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 23 Oct 2019 04:21:39 +0000 Gerrit-HasComments: Yes
