Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service 
queue
......................................................................


Patch Set 1:

(7 comments)

The high level discussion Tim brings up is important, but also prefetching some 
comments on the specifics of the implementation.

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.h@43
PS1, Line 43: MemTracker* mem_tracker
it would be good to document these parameters. Is this the mem_tracker that the 
pool will use to account queued RPCs, or what?  Actually, I think it's the 
mem-tracker associated with 'service', right? How about calling it 
service_mem_tracker to make that clear?


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.cc@150
PS1, Line 150: or
, otherwise


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.cc@152
PS1, Line 152: std::
nit: names.h should take care of that


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

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr.h@127
PS1, Line 127: mem_tracker
document. and this is the service's mem tracker right? if so, maybe 
service_mem_tracker?


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.h
File be/src/runtime/krpc-data-stream-mgr.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.h@234
PS1, Line 234: handed over to DataStreamService
is that correct? Isn't incoming_request_tracker the DataStreamService's 
mem_tracker?


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

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.cc@175
PS1, Line 175: Release
I guess we were using the "Local" versions before since we were just 
transferring the memory between siblings.  Why is it safe to now release it all 
the way and then consume (leaving a window of it being unaccounted)?


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.h
File be/src/service/data-stream-service.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.h@58
PS1, Line 58:   MemTracker* mem_tracker() { return mem_tracker_.get(); }
does that need to be public?



--
To view, visit http://gerrit.cloudera.org:8080/9282
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Feb 2018 21:50:44 +0000
Gerrit-HasComments: Yes

Reply via email to