Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14183 )
Change subject: IMPALA-8830: Fix executor group assignment of coordinator only queries ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/14183/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/14183/2/be/src/scheduling/admission-controller.cc@625 PS2, Line 625: string* rejection_reason) { > nit: drop std:: in .cc files Done http://gerrit.cloudera.org:8080/#/c/14183/2/be/src/scheduling/admission-controller.cc@1122 PS2, Line 1122: || ClusterMembershipMgr::GetEmptyExecutorGroup() == executor_group); > Would this group ever be unhealthy? the CoordintorOnlyExecutorGroup is empty so its always unhealthy technically. The only way for this to be actually unhealthy is for the coordinator (which is running this query goes down and thats is impossible) http://gerrit.cloudera.org:8080/#/c/14183/2/be/src/scheduling/admission-controller.cc@1127 PS2, Line 1127: VLOG(3) << "Scheduling for executor group: " << group_name << " with " > I wonder if this log statement will be confusing when scheduling on the coo I think if we communicate that the group named "coordinator-only-group" is a special group, it should be ok, since we'll be in the same boat when we look at the query profile and it prints out the "Executor Group" as "coordinator-only-group". One way to make sure that this name is special is by reserving this name and not allowing any exec groups to have this listed as their group name, either by rejecting it at startup or just ignoring it. http://gerrit.cloudera.org:8080/#/c/14183/2/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/14183/2/be/src/scheduling/cluster-membership-mgr.h@177 PS2, Line 177: /// Returns a pointer to the static empty group reserved for scheduling coord only > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/14183/2/be/src/scheduling/cluster-membership-mgr.h@220 PS2, Line 220: > I understand why it is like this, but I think it's a problem that the name I agree. Done. http://gerrit.cloudera.org:8080/#/c/14183/2/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/14183/2/be/src/scheduling/cluster-membership-mgr.cc@48 PS2, Line 48: static const string EMPTY_GROUP_NAME("empty group (using coordinator only)"); > This could be "empty group (using coordinator)" (or just the empty string)? "empty group (using coordinator)" => this seems good to me as the empty one can be confusing. I dont really have a preference here, other than having a name that communicates implicitly that it means that the query will only run on the coordinator, and your suggestion captures this pretty perfectly. http://gerrit.cloudera.org:8080/#/c/14183/2/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/14183/2/tests/custom_cluster/test_executor_groups.py@97 PS2, Line 97: expected_group = "E > nit: expected_group ? Done -- To view, visit http://gerrit.cloudera.org:8080/14183 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fe098032744aa20bbbe4faddfc67e7a46ce03d5 Gerrit-Change-Number: 14183 Gerrit-PatchSet: 3 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@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-Comment-Date: Tue, 24 Sep 2019 00:30:54 +0000 Gerrit-HasComments: Yes
