Lars Volker 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:

(6 comments)

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.

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG@15
PS1, Line 15: previou
nit: typo


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

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h@133
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 
here?


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

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/exec-env.cc@304
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 
ctor.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/exec-env.cc@305
PS1, Line 305: data_svc_
this one, too


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


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

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.cc@46
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 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: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Feb 2018 20:33:43 +0000
Gerrit-HasComments: Yes

Reply via email to