Lars Volker has posted comments on this change. ( )

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

Patch Set 3:


> 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.

I'm curious how do others feel about this?

> However, if we want to go with ExecEnv, we should probably remove some of the 
> injected dependencies as well to keep it consistent.
File be/src/rpc/rpc-mgr-test-base.h:
PS1, Line 133:   // Takes over ownership of the newly created 'service' which 
needs to have a lifetime
> The RpcMgr no longer owns the rpc service. RpcMgr::Shutdown() will call rpc
Shouldn't the RpcMgr own the services then?
File be/src/runtime/
PS1, Line 83:   mem_tracker_.reset(new MemTracker(-1, "Data Stream Manager 
Deferred RPCs",
> It seems the common pattern is to have the MemTracker inside the owning obj
That makes sense to me, thanks for the explanation.

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: 3
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Thu, 15 Feb 2018 03:23:51 +0000
Gerrit-HasComments: Yes

Reply via email to