Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4856: Port data stream service to KRPC ......................................................................
Patch Set 3: (17 comments) http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/rpc/rpc.h File be/src/rpc/rpc.h: PS3, Line 147: func Having the names 'func', 'cb' and 'cb_wrapper' all close by each other makes some of this slightly more complicated to read. I would opt for renaming 'func' to 'rpc_method' or something, so that it's crystal clear that it may not be a callback itself. PS3, Line 153: std::move(params_) Does this move mean that the params_ member is invalid after this call? If so, it would be good to add a comment where it's declared. PS3, Line 327: cb Maybe name this 'completion_cb'. http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: PS3, Line 389: Copying proto_filter here ensures that its lifetime will last at least until this : // callback completes. : auto cb = [proto_filter] Why wouldn't a move capture ensure the same thing? http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS3, Line 1207: // Do an explicit copy of the directory: 'params' may have come from an RPC so we : // can't assume ownership of its directory. : bloom_filter_ = : make_unique<ProtoBloomFilter>(params.header, params.directory); I would add this to the commit message. This means we would take double the memory cost every time we merge filters right? http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/data-stream-recvr.cc File be/src/runtime/data-stream-recvr.cc: PS3, Line 280: blocked_senders_.front() Is this a right way to dispose a unique_ptr? http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/data-stream-sender.cc File be/src/runtime/data-stream-sender.cc: PS3, Line 58: DECLARE_int32(datastream_sender_timeout_ms); Not used. PS3, Line 60: DECLARE_int32(state_store_subscriber_port); Not used. PS3, Line 133: AtomicInt64 num_data_bytes_sent_ No one really calls GetNumDataBytesSent() (except from our BE test). So, I'm not sure we're gaining anything by making this atomic. Not a big deal, but it might be cheaper to leave it as an int64. Your call though. PS3, Line 148: self_ A reader of this code might not immediately understand why this class needs to always have a reference to itself. It would be good to explicitly mention this member name in the header where the explanation is given. PS3, Line 170: self_ = shared_from_this(); Why is this set in Init()? Wouldn't it ideally be set it in the constructor? PS3, Line 175: auto proto_batch Just want to make sure that this will increase the shared_ptr refcount? It should because it will make an underlying copy of the pointer, but I just want to make sure. PS3, Line 203: cb Prefer a more descriptive name "rpc_completion_cb" or something similar. Line 336: batch_.reset(); DCHECK(self_.get()) http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/row-batch.cc File be/src/runtime/row-batch.cc: Line 216: output_batch->header.set_compression_type(THdfsCompression::LZ4); Do you think it would be good to add a comment why we don't free the 'tuple_data' buffer here? Presumably so we can reuse the memory when the RowBatch is recycled? http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: PS3, Line 73: == THdfsCompression::LZ4 != THdfsCompression::NONE Functionally same, more readable. PS3, Line 441: FlushMode flush_ = FlushMode::NO_FLUSH_RESOURCES Why not initialize this and the other args below with the default values in the constructor member initialization list? -- To view, visit http://gerrit.cloudera.org:8080/7103 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia66704be7a0a8162bb85556d07b583ec756c584b Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
