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 1: (5 comments) I think it would be good to unify the ways in which we pass the local BE desc into the scheduler. With this change we pass it both as a BE desc and as the coordinator-only group. We could stay with the old approach and pass an ExecutorConfig with an empty executor group, and then build the coordinator-only group as we did before in the scheduler. I don't see an easy way to pass it with a coordinator-only group when also using a regular executor group for scheduling. http://gerrit.cloudera.org:8080/#/c/14183/1/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/14183/1/be/src/scheduling/cluster-membership-mgr.h@93 PS1, Line 93: ExecutorGroup local_coord_only_group = ExecutorGroup("coordinator-only-group"); It seems that so far we have created a group with the coordinator on-demand in the scheduler. Having a special group inside executor_groups could be a cleaner approach. Can we move this group there? http://gerrit.cloudera.org:8080/#/c/14183/1/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/14183/1/be/src/scheduling/scheduler.cc@143 PS1, Line 143: bool exec_at_coord = (fragment.partition.type == TPartitionType::UNPARTITIONED); Should exec_at_coord now be replaced with IsCoordinatorOnlyQuery()? http://gerrit.cloudera.org:8080/#/c/14183/1/be/src/scheduling/scheduler.cc@465 PS1, Line 465: ExecutorGroup coord_only_executor_group("temp-coordinator-only-group"); This should now use the coordinator-only group, no? http://gerrit.cloudera.org:8080/#/c/14183/1/tests/custom_cluster/test_executor_groups.py File tests/custom_cluster/test_executor_groups.py: http://gerrit.cloudera.org:8080/#/c/14183/1/tests/custom_cluster/test_executor_groups.py@29 PS1, Line 29: schduled nit: typo http://gerrit.cloudera.org:8080/#/c/14183/1/tests/custom_cluster/test_executor_groups.py@29 PS1, Line 29: ideally Why "ideally" instead of "that gets scheduled". -- 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: 1 Gerrit-Owner: 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: Thu, 05 Sep 2019 22:54:44 +0000 Gerrit-HasComments: Yes
