Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/8914 )
Change subject: IMPALA-6193: Track memory of incoming data streams ...................................................................... Patch Set 3: (10 comments) Thanks for the comments, please see PS3. http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG@9 PS2, Line 9: change > change Done http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG@10 PS2, Line 10: > nit, will help to know which mem tracker this is counted against, in case s Done http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG@11 PS2, Line 11: nager. There we > another global tracker labelled "Rpc Memory" Done http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG@13 PS2, Line 13: register > nit: receiver, Done http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG@13 PS2, Line 13: e the receiver, : memory > nit, just to make it more clearer what the term instance means here: its fr Done http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/rpc/rpc-mgr.h@102 PS2, Line 102: Status Init() WARN_UNUSED_RESULT; > This seems like the wrong abstraction to me. I would expect the caller of R I changed it as you suggested. http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/rpc/rpc-mgr.cc@29 PS2, Line 29: > Duplicated line. Bad rebase ? Done http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/exec-env.cc@325 PS2, Line 325: t to track > "Rpc Memory" seems vague. Also, please see comments in rpc-mgr about how we I changed it to have one pool per service. I kept separate trackers for memory in the inbound call queue and the early sender queue. Let me know if you prefer to use the same tracker for both. http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/krpc-data-stream-mgr.cc File be/src/runtime/krpc-data-stream-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/krpc-data-stream-mgr.cc@203 PS2, Line 203: TransmitDataCtx payload(request, response, rpc_context); : Status status = payload.Init(); > Not too thrilled about this extra malloc here. Can we avoid it altogether ? I removed it so that the normal path of not queueing it does not have the malloc. http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/krpc-data-stream-mgr.inline.h File be/src/runtime/krpc-data-stream-mgr.inline.h: http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/krpc-data-stream-mgr.inline.h@25 PS2, Line 25: : : const TransmitDataRequestPB* TransmitDataCtx::request() const { : DCHECK(initialized_); : return request_; : } : : TransmitDataResponsePB* TransmitDataCtx::response() { : DCHECK(initialized_); : return response_; : } : : kudu::rpc::RpcContext* TransmitDataCtx::rpc_context() { : DCHECK(initialized_); : return rpc_context_; : } : : const kudu::Slice& TransmitDataCtx::tuple_offsets() const { : DCHECK(initialized_); : return tuple_offsets_; : } : : const kudu::Slice& TransmitDataCtx::tuple_data() const { : DCHECK(initialized_); : return tuple_data_; : } : : int64_t TransmitDataCtx::serialized_size() const { : DCHECK(initialized_); : return serialized_size_; : } : : int64_t TransmitDataCtx::deserialized_size() const { : DCHECK(initialized_); : return deserialized_size_; : } : : void TransmitDataCtx::RespondStatus(const Status& status) { : status.ToProto(response_->mutable_status()); : rpc_context_->RespondSuccess(); : } : : void EndDataStreamCtx::RespondStatus(const Status& status) { : status.ToProto(response_->mutable_status()); : rpc_context_->RespondSuccess(); : } > Most of these look like one liners which can be moved to krpc-data-stream-m My intention was to make the code safer by adding the DCHECKs, which makes them 2 lines. I'm still happy to move them to the header as such, let me know if you prefer that. -- To view, visit http://gerrit.cloudera.org:8080/8914 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2df1204d2483313a8a18e5e3be6cec9e402614c4 Gerrit-Change-Number: 8914 Gerrit-PatchSet: 3 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Comment-Date: Thu, 04 Jan 2018 17:24:44 +0000 Gerrit-HasComments: Yes
