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 1: (5 comments) > (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. We cant pass an executor group because further down the code path we expect an exec group associated with an admitted schedule to have non-zero executors as we use the group size for making admission decisions. 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 I thought of that but decided against it since the whole premise of an executor group is that it is composed of executors only, which is why i called this a "pseudo group". Also adding a special group in the ExecutorGroups list means there will always be a healthy group with ""coordinator-only-group"" being a reserved group name (not sure how we can enforce that). moreover, this group wont be bound by the group_name->resource_pool mapping rule. Basically a bunch of special handling will be required for that special exec group and having that implicit in a regular vector of ExecutorGroup objects would be weird. I initially thought of creating ExecutorGroups class that encompasses these special cases, and abstract away some of that logic there but if I keep going that route I would end up with something like the all encompassing Snapshot struct, so I just added the pseudo group to it. I agree all this is a bit messy so open to suggestions. Maybe we can just pass the full snapshot and a list of exec groups for the resource pool to the scheduler and handle some of the messiness there (handling the IsCoordinatorOnlyQuery, temp-coordinator-only-group parts, Scheduler::ExecutorConfig parts). 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()? the exec_at_coord used here is at the fragment level to identify if a fragment is to be scheduled on the coordinator, whereas IsCoordinatorOnlyQuery is for the whole query that also makes sure the plan contains only one fragment. 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? Unfortunately, the way scheduling works, this temp group is required because ComputeScanRangeAssignment is done for all queries and "non-coordinator only" queries would not have coordinator-only-group in their ExecutorConfig 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 will fix in the next patch 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". will fix in the next patch -- 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: 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 23:45:53 +0000 Gerrit-HasComments: Yes
