impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6193: Track memory of incoming data streams
Date Tue, 09 Jan 2018 02:26:09 GMT
Michael Ho 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:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/rpc/rpc-mgr.h@165
PS3, Line 165:   /// Passed to new services.
             :   MemTracker* mem_tracker_;
Is this still needed ?


http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/exec-env.cc@343
PS3, Line 343: 
nit: extra blank line


http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8914/3/be/src/runtime/krpc-data-stream-mgr.cc@175
PS3, Line 175: ctx->serialized_size()
Wouldn't the majority of the changes be not needed if we add a new interface ctx->GetTransferSize()
instead ?

IMHO, this may be a more generic interface which may be used by other RPCs to be added in
the future.

Sorry for not providing better advice earlier.


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();
            : }
> My intention was to make the code safer by adding the DCHECKs, which makes 
These changes may not be needed with the TransferSize() changes.



-- 
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: Tue, 09 Jan 2018 02:26:09 +0000
Gerrit-HasComments: Yes

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