Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13740 )
Change subject: IMPALA-7486: Add specialized estimation scheme for dedicated coordinators ...................................................................... Patch Set 15: (15 comments) Mostly nits and readability stuff http://gerrit.cloudera.org:8080/#/c/13740/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13740/15//COMMIT_MSG@21 PS15, Line 21: backends Maybe clarify whether this included coordinators? These days I find it easy to get confused by the team backend in changes where the distinction matters http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller-test.cc@106 PS15, Line 106: nit: extra line http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller-test.cc@796 PS15, Line 796: PerBackendExecParams* per_backend_exec_params = pool_.Add(new PerBackendExecParams()); : BackendExecParams* coord_exec_params = pool_.Add(new BackendExecParams()); : coord_exec_params->admit_mem_limit = 512 * MEGABYTE; : coord_exec_params->contains_coord_fragment = true; : coord_exec_params->thread_reservation = 1; : const string coord_host_name = Substitute("host$0", 1); : TNetworkAddress coord_addr = MakeNetworkAddress(coord_host_name, 25000); : const string coord_host = TNetworkAddressToString(coord_addr); : per_backend_exec_params->emplace(coord_addr, *coord_exec_params); : BackendExecParams* backend_exec_params = pool_.Add(new BackendExecParams()); : backend_exec_params->admit_mem_limit = GIGABYTE; : backend_exec_params->thread_reservation = 1; : const string exec_host_name = Substitute("host$0", 2); : TNetworkAddress exec_addr = MakeNetworkAddress(exec_host_name, 25000); : const string exec_host = TNetworkAddressToString(exec_addr); : per_backend_exec_params->emplace(exec_addr, *backend_exec_params); : query_schedule->set_per_backend_exec_params(*per_backend_exec_params); Have you considered factoring some of this in to a helper function to improve readability? http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller.h@773 PS15, Line 773: executor backends nit: "executors" should be fine, we use that term pretty much everywhere else http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller.h@774 PS15, Line 774: among them What does "among them" mean here? "largest initial reservation of any executor"? http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller.h@785 PS15, Line 785: coordinator backend nit: just "coordinator" http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller.h@806 PS15, Line 806: /// Based on whether the query represented by 'schedule' is being admitted or not, it nit: Simplify comment, e.g. "Updates the memory admitted for each host in 'schedule'. If 'is_admitting' is true, the memory admitted is increased, otherwise it is decreased." http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller.h@943 PS15, Line 943: FRIEND_TEST(AdmissionControllerTest, RejectImmediately); This one looks like it can be removed. I think I might have introduced this when I rebased the change. :( http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller.cc@480 PS15, Line 480: non_coordinator executor_mem_to_admit? http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller.cc@1146 PS15, Line 1146: FLAGS_is_executor I had to check the c'tor of QuerySchedule to remind myself why we pass this here. Maybe prefix with /* is_dedicated_coord=*/ or make a local variable for it first? http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/admission-controller.cc@1146 PS15, Line 1146: nit: extra space http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/query-schedule.cc File be/src/scheduling/query-schedule.cc: http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/scheduling/query-schedule.cc@277 PS15, Line 277: GetDedicatedCoordMemoryEstimate() : nit: join with next line (unless clang-format accepts this) http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/service/impala-http-handler.cc@938 PS15, Line 938: mem_limit nit: executor_mem_limit? http://gerrit.cloudera.org:8080/#/c/13740/15/be/src/service/impala-http-handler.cc@940 PS15, Line 940: coord_backend_mem_limit nit: just coord_mem_limit? http://gerrit.cloudera.org:8080/#/c/13740/15/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: http://gerrit.cloudera.org:8080/#/c/13740/15/fe/src/main/java/org/apache/impala/planner/Planner.java@76 PS15, Line 76: A value of 100 MB would : // be ideal considering nit: "We pick a value of 100MB because it is trivial..." -- To view, visit http://gerrit.cloudera.org:8080/13740 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2b94e7293b91dec8a18491079c34923eadd94b21 Gerrit-Change-Number: 13740 Gerrit-PatchSet: 15 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 26 Jul 2019 17:26:37 +0000 Gerrit-HasComments: Yes
