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