Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14425 )
Change subject: KUDU-2971 p1: add subprocess module ...................................................................... Patch Set 2: > 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. Yeah, I chatted briefly about this with Hao on Slack and alluded to something similar in my comments on the Java patch. I think for concurrent calls, we'd want an ID field like what we have in the RPC headers. -- 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 05:59:02 +0000 Gerrit-HasComments: No
