Matthew Jacobs has posted comments on this change.

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

Patch Set 9:


Focused mostly where I left off previously, from the Coordinator.
File be/src/runtime/

Line 229: 
.. total # fragments -1?
File be/src/runtime/

PS9, Line 227:   /// Index of 'this' in Coordinator::fragment_instance_states_
             :   int state_idx_;
not used

PS9, Line 485: int32_t
inconsistent use of int/int32_t

PS9, Line 486:  
nit: space

PS9, Line 488: num_remaining_fragment_instances_ = num_remote_instances;
move this above near other usage of num_remote_instances

PS9, Line 494:   // to keep things simple, make async Cancel() calls wait until 
plan fragment
             :   // execution has been initiated, otherwise we might try to 
cancel fragment
             :   // execution at Impala daemons where it hasn't even started
I don't understand why this comment belongs here. Am I missing something or 
should it be moved/removed?

PS9, Line 542: FinishQueryStartup
Seems weird that we wouldn't have to call a function called 
'FinishQueryStartup()' when there are no remote finstances. I'd suggest 
changing the name but I guess it's fine since I assume this will go away with 
Henry's upcoming change.

PS9, Line 637:     // apparently this was never called for the coordinator 
             :     //exec_state->ComputeTotalSplitSize(
             :         //rpc_params.fragment_instance_ctx.per_node_scan_ranges);
Why leave this as a comment? I'd imagine it will be called for the coord 
fragment after Henry's change to make the coord fragment start like any other.

PS9, Line 645: GetInstanceIdx(coord_state->fragment_instance_id())
this and other cases where we index into fragment_instance_states_ would be 
nice to use FragmentInstanceState::state_idx_ 

It looks to me like state_idx_ is not set or used yet. If we're not going to 
use it, let's remove it.

PS9, Line 735: // DCHECK(filter_mode_ == TRuntimeFilterMode::OFF);
why is this a comment?

PS9, Line 746: instance_state_idx
same thing about using state_idx_

PS9, Line 1555: const DebugOptions* debug_options
It'd be nice to avoid passing these through the fn. I don't see any reason they 
can't just be set on FragmentInstanceState*.

PS9, Line 1564:   int instance_state_idx = 
              :   FragmentInstanceState* exec_state = 
same thing about the state_idx_ parameter

PS9, Line 1934: &fragment_profiles_[instance_state->fragment_id()];
was removing the bounds checks intentional? Here and below.
File be/src/runtime/coordinator.h:

Line 101: 
nit: extra line
File be/src/runtime/coordinator.h:

PS9, Line 547: Also sets up and
             :   /// registers the state for the single coordinator fragment 
Looks to me like it sets up the state for all non-coordinator fragment 
File be/src/scheduling/

PS9, Line 53: void ResourceResolver::GetResourceHostport(const TNetworkAddress& 
            :     TNetworkAddress* dest) const {
            :   if (ExecEnv::GetInstance()->is_pseudo_distributed_llama()) {
            :     auto entry = impalad_to_dn_.find(src);
            :     if (entry != impalad_to_dn_.end()) {
            :       *dest = entry->second;
            :     } else {
            :       *dest = TNetworkAddress();
            :     }
            :   } else {
            :     dest->hostname = src.hostname;
            :     dest->port = 0;
            :   }
I take it you'll rebase on the llama removal patch?
File be/src/scheduling/query-schedule.h:

PS9, Line 46: boost::unordered_map
Do you know if we're avoiding std::unordered_map?

PS9, Line 176:   
extra spaces
File be/src/scheduling/

PS4, Line 541:       // total_assigned_bytes won't hit total_size until 
everything is assigned
             :       while (params_idx < params_list.size()
> i'm not sure what to add.
Ignore this, I misread this code before.
File be/src/scheduling/

PS9, Line 942: // TODO-MT: call AdmitQuery()
any reason you can't do this now?
File be/src/service/

PS9, Line 148: VLOG_QUERY
This might be noisy for QUERY

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I240445199e22f009f4e72fdb8754eb8d77e3d680
Gerrit-PatchSet: 9
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-Reviewer: Mostafa Mokhtar <>
Gerrit-HasComments: Yes

Reply via email to