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

Reply via email to