impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6193: Track memory of incoming data streams
Date Thu, 04 Jan 2018 17:24:44 GMT
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 <lv@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Jan 2018 17:24:44 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message