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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 12 Feb 2018 21:50:44 +0000 Gerrit-HasComments: Yes
