Lars Volker 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 2: (6 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: std::string* rejection_reason) { nit: drop std:: in .cc files http://gerrit.cloudera.org:8080/#/c/14183/2/be/src/scheduling/admission-controller.cc@1122 PS2, Line 1122: || ClusterMembershipMgr::GetCoordintorOnlyExecutorGroup() == executor_group); Would this group ever be unhealthy? 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 coord-only group 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@220 PS2, Line 220: coord_only_exec_group_ I understand why it is like this, but I think it's a problem that the name does not reflect that it's empty. If you see "coord_only_exec_group_" you pretty much expect that it only contains the coordinator. I think it'd be better to call it "empty_exec_group" and explain why it is like that in places where we use it. 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 COORD_ONLY_GROUP_NAME("coordinator-only-group"); This could be "empty group (using coordinator)" (or just the empty string)? 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_in_profile nit: expected_group ? -- 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: 2 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: Mon, 23 Sep 2019 20:32:21 +0000 Gerrit-HasComments: Yes
