Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-3902: Scheduler improvements for running multiple 
fragment instances on a single backend

Patch Set 4:

File be/src/runtime/coordinator.h:

PS4, Line 218:   const TNetworkAddress local_addr_;
> Can you use ExecEnv::backend_address()? Or find some way to avoid the dupli

PS4, Line 297: Wait
> nit: Wait()

PS4, Line 388: fragment_instance_state_idxs
> Is this referencing the "fragment id ("? If so, please say
it's not, it's an index into fragment_instance_states_. i thought that would be 
clear from the name.

added comment.
File be/src/scheduling/

PS4, Line 121:   std::sort(fragments.begin(), fragments.end(),
             :       [](const TPlanFragment* a, const TPlanFragment* b) { 
return a->id < b->id; });
> shouldn't these come in an expected order?
this adds an extra level of robustness, otherwise i have to make this ordering 
guarantee part of the interface.

i don't expect this to have any performance impact.

PS4, Line 346: NULL
> nullptr
File be/src/scheduling/query-schedule.h:

Line 97:   std::map<PlanNodeId, int> per_exch_num_senders;
> any benefit from using map over unordered_map here?
it makes it easier to assign to thrift structs, and this isn't 

PS4, Line 173: NULL
> nullptr

Line 174:   const TPlanFragment* GetCoordFragment() const; 
> nit: trailing whitespace

Line 213:   MtFragmentExecParams* GetFragmentExecParams(FragmentId id) {
> If you need non-const access, isn't it more idiomatic to return a non-const
we typically return pointers for mutable structures (and const refs for 
immutable structures).

PS4, Line 219: NULL
> nullptr

Line 307: 
> comment, e.g. what state in MtFragmentExecParams gets initialized here vs. 
File be/src/scheduling/

PS4, Line 384: auto
> auto only for iterators
this is a map iterator

PS4, Line 414: depth-first traversal
> isn't this an implementation detail of the other MtComputeFragmentExecParam

PS4, Line 425: f
> can you change f indexes here to match the code? it's explained as if it st
moved comment

PS4, Line 433:       const TDataStreamSink& sink = 
             :       DCHECK(
             :           sink.output_partition.type == 
             :             || sink.output_partition.type == 
             :             || sink.output_partition.type == 
> move this closer to where it's used on l453

PS4, Line 470: MtComputeFragmentExecParams
> Can we be sure that recursion depth will not be a problem here?

Line 524:     // try to load-balance scan ranges by assigning just beyond the
> nit: wrap at 90 characters

PS4, Line 541:       while (params_idx < params_list.size()
             :           && total_assigned_bytes < threshold_total_bytes) {
> It's not obvious to me why this will always assign all params. If the last 
i'm not sure what to add.

for the last fragment, threshold_total_bytes == total_size, so this won't get 
exceeded. if total_assigned_bytes == total_size, by definition there can't be 
any params left.

let me know how i can clarify.

Line 695:     for (int j = 0; j < params.hosts.size(); ++j, 
++num_fragment_instances) {
> you could call reserve() before this line
i'll leave the old code as-is.

PS4, Line 933:     if (!FLAGS_disable_admission_control) {
             :     }
             :     if (!FLAGS_enable_rm) return Status::OK();
             :     string user = 
             :     if (user.empty()) user = "default";
             :     schedule->PrepareReservationRequest(resolved_pool, user);
             :     const TResourceBrokerReservationRequest& reservation_request 
             :         schedule->reservation_request();
             :     if (!reservation_request.resources.empty()) {
             :       Status status = resource_broker_->Reserve(
             :           reservation_request, schedule->reservation());
             :       if (!status.ok()) {
             :         // Warn about missing table and/or column stats if 
             :         const TQueryCtx& query_ctx = 
             :         if(!query_ctx.__isset.parent_query_id &&
             :             query_ctx.__isset.tables_missing_stats &&
             :             !query_ctx.tables_missing_stats.empty()) {
             :         }
             :         return status;
             :       }
             :       RETURN_IF_ERROR(schedule->ValidateReservation());
             :       AddToActiveResourceMaps(*schedule->reservation(), coord);
             :     }
> while the llama related code will be removed soon, I think you should still

Line 1104:       it = backend_map_.insert(make_pair(ip, BackendList())).first;
> BackendConfig has been moved to an own file in asf/master, and this code ha
i'm sure it will, but i'm holding off on rebases for now.
File be/src/scheduling/simple-scheduler.h:

PS4, Line 489:  
> nit: whitespace, wrap at 90 characters

PS4, Line 492: mt_fragment_exec_params_
> Is there a way around referring to a private member of the schedule here?
this class deals with populating private members of that class, so no.

PS4, Line 499: MtComputeFragmentExecParams
> The separation between what structures are set/updated in QuerySchedule and
QuerySchedule is really just a helper class which functions as a container for 
scheduling data. some of that data comes from the initial TExecRequest, some is 
created by the scheduling logic. i moved all static setup functions as well as 
purely mechanical functions into QuerySchedule (such as figuring out the number 
of remote fragment instances), but i think the actual scheduling logic should 
stay in the scheduler.

PS4, Line 502: depth-first traversal
> Is this implementation-detail relevant for the caller?
it's needed for understanding the correctness - the bottom-up process relies on 
having scheduling data for the children before scheduling the parent fragment.

PS4, Line 504: MtComputeFragmentExecParams
> It'd be nice to have a different and more specific name, e.g. MtCreateFInst
i would argue that the current name is more specific, because it does more than 
just the FInstanceExecParams. i could call this MtComputeMtFragmentExecParams, 
but didn't think that was necessary. also, keep in mind that the 'mt' prefixes 
are only temporary.
File be/src/service/

PS4, Line 439:   // TODO-MT: if exec_request_.query_options.mt_dop isn't set, 
set it to the pool
             :   // default, if any
> This should already happen in impala-{hs2,beeswax}-server when the query co

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I240445199e22f009f4e72fdb8754eb8d77e3d680
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

Reply via email to