Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4856: Port ImpalaInternalService to KRPC ......................................................................
Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/5888/3/be/src/runtime/data-stream-mgr.h File be/src/runtime/data-stream-mgr.h: Line 77: /// After the first batch has been received, the sender continues to send batches, one at > No, for a couple of reasons: follow-on questions: - should the queue be sized in terms of bytes or rows/row batches? the latter would probably lead to steadier behavior - regarding point 2: with correct flow control, i don't see how the receiver stalling would still allow the senders to fill up the queue. let's talk in person tomorrow, rather than a back & forth over comments. Line 84: /// notified when the batch has been processed, or added to the batch queue, > I'm not sure which part you're referring to - but yes, in general this is h let's discuss tomorrow, i'm wondering if this feature can be avoided. Line 131: /// is reached, the RPC will be considered failed, and the channel and sender will be > In the queued batch case, we could look to see if the queue had any more ca i think we should size the queue in terms of batches, not total bytes. the sender itself can keep track of total bytes. Line 245: /// If the stream would exceed its buffering limit as a result of queuing this batch, > I feel that rvalues do make ownership explicit. You can't pass a non-tempor i think this is inferior to passing a pointer to the context and stipulating that the caller becomes the owner. also, move() is still more expensive than passing a pointer, and also more opaque (you don't know what's getting copied, unless you know the details of every field in the struct). i really don't see any upside to a move() here. http://gerrit.cloudera.org:8080/#/c/5888/5/be/src/runtime/data-stream-mgr.h File be/src/runtime/data-stream-mgr.h: Line 124: /// execution. Senders do not block in TransmitData() RPCs, and may be cancelled at any what do you mean, they don't block in transmitdata rpcs? they have to wait for a response, no? Line 133: /// stream to continue correct operation. where did the 120s come from again? -- To view, visit http://gerrit.cloudera.org:8080/5888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I95229290566a8ccffd80ed2d74c1c57cf1479238 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Anonymous Coward #168 Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
