Lars Volker has posted comments on this change. ( )

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

Patch Set 1:


I went through the change and found that I would prefer to not access 
dependencies through the ExecEnv singleton, but instead inject them through the 
ctors and Init(). That way it feels easier to me to follow the code. I don't 
feel strongly about it though.

However, if we want to go with ExecEnv, we should probably remove some of the 
injected dependencies as well to keep it consistent.
Commit Message:
PS1, Line 15: previou
nit: typo
File be/src/rpc/rpc-mgr-test-base.h:
PS1, Line 133:   // Takes over ownership of the newly created 'service'.
Can you explain why this method is needed? Could you just pass in a pointer 
File be/src/runtime/
PS1, Line 304:     RETURN_IF_ERROR(data_svc_->Init(rpc_mgr_.get()));
The RpcMgr has a getter in the ExecEnv, too, so you could remove it from the 
PS1, Line 305: data_svc_
this one, too
File be/src/runtime/
PS1, Line 83:   mem_tracker_.reset(new MemTracker(-1, "Data Stream Manager 
Deferred RPCs",
I found this easier to follow when the MemTrackers were created in ExecEnv with 
the rest of the trackers. I don't feel strongly about it though.
File be/src/service/
PS1, Line 46: DataStreamService::DataStreamService(RpcMgr* rpc_mgr)
It seems odd to pass the rpc_mgr into both the ctor and Init().

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Tue, 13 Feb 2018 20:33:43 +0000
Gerrit-HasComments: Yes

Reply via email to