Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8023 )
Change subject: IMPALA-4856: Port data stream service to KRPC ...................................................................... Patch Set 9: (21 comments) http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-mgr.h File be/src/runtime/krpc-data-stream-mgr.h: http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-mgr.h@435 PS8, Line 435: 'num_requ > 'num_request' requests Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-mgr.cc File be/src/runtime/krpc-data-stream-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-mgr.cc@62 PS8, Line 62: ds, C > how was that chosen? do we have a test case that causes this queue to fill Just a large enough number to reduce the chance of the queue filling up. Yes, need to come up with a test case to fill up this queue. http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-mgr.cc@109 PS8, Line 109: // Let the receiver take over the RPC payloads of early senders and process them : // asynchronously. : for (unique_ptr<TransmitD > this comment seems out of place. this is more an implementation detail of t Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.h File be/src/runtime/krpc-data-stream-recvr.h: http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.h@129 PS8, Line 129: n > and start a deserialization task to process it asynchronously. Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.h@130 PS8, Line 130: ask to p > this "transfer" is in the oppose direction of how our "Transfer" methods us Renamed to TakeOverEarlySender(). http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.h@197 PS8, Line 197: eive > cpu time or wall time? wall-clock time. http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.h@200 PS8, Line 200: _; > same question wall-clock time. http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@67 PS8, Line 67: data > the resources Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@72 PS8, Line 72: imit, the > RPC state Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@73 PS8, Line 73: nto 'deferred_ > we shouldn't normally refer to private fields in public class comments, but Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@87 PS8, Line 87: void TakeOverEarlySender(std::unique_ptr<TransmitDataCtx> ctx); > same comment as recvr header comment. Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@195 PS8, Line 195: // cur_batch_ must be replaced with the > I don't think we need this loop. see other comments in this function. Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@197 PS8, Line 197: atch = nullptr; > nit: consider swapping the order of these so that the fast case comes first Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@210 PS8, Line 210: } > nit: i think we could do without some of the blank lines in this method to I leave a blank line between each loop exiting conditions. http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@211 PS8, Line 211: > Actually, I think the loop exiting condition is not quite right, which led There is an invariant that EOS cannot be sent by a sender when there is outstanding TransmitData() RPC so we should be able to get by by just checking for the termination condition of: (num_remaining_senders_ == 0 && batch_queue_.empty()) http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@218 PS8, Line 218: > given that we just checked the other two loop exit conditions, isn't this d Converted to DCHECK(). http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@229 PS8, Line 229: d > to parallelize the CPU bound deserialization work. Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@239 PS8, Line 239: } > once you get rid of the loop, I think you'll be able to eliminate this unlo Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@389 PS8, Line 389: status.ToProto(ctx->response->mutable_status()); > at this point, 'ctx' effectively takes ownership, right? we should add a co Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-recvr.cc@406 PS8, Line 406: const RowBatchHeaderPB& header = ctx->request->row_batch_header(); : AddBatchWork(batch_size, header, tuple_offsets > nit: let's reorder these two lines since num_deferred_batches_ is counting Done http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/8023/8/be/src/runtime/krpc-data-stream-sender.cc@434 PS8, Line 434: resp_.Clear(); > May want to call resp_.Clear() too. Done -- To view, visit http://gerrit.cloudera.org:8080/8023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic0b8c1e50678da66ab1547d16530f88b323ed8c1 Gerrit-Change-Number: 8023 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Tue, 07 Nov 2017 21:49:49 +0000 Gerrit-HasComments: Yes
