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 10:

File be/src/runtime/

PS3, Line 713: 
> ?

PS3, Line 1654:  (!rpc_status.ok()) {
> Not sure what that means - you've added code to explicitly skip the coordin
the coordinator previously didn't have an associated FragmentInstanceState. i 
added that, and am now skipping it in this path, since it's not a remote 
File be/src/runtime/

PS9, Line 203:  const
> I think what the original comment was trying to get at was that the returne

PS9, Line 227:   int per_fragment_instance_idx_;
> not used

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

PS9, Line 486: 
> nit: space

PS9, Line 488: progress_.Init(str, schedule_.num_scan_ranges());
> move this above near other usage of num_remote_instances

PS9, Line 494: 
             :   bool has_coordinator_fragment = schedule_.GetCoordFragment() 
!= NULL;
             :   if (has_coordinator_fragment) {
> I don't understand why this comment belongs here. Am I missing something or
it's a comment on the lock acquisition. added a blank line.

PS9, Line 542: ken on the coordin
> Seems weird that we wouldn't have to call a function called 'FinishQuerySta

PS9, Line 637:   }
             :   executor_.reset(new PlanFragmentExecutor(
             :       exec_env_, PlanFragmentExecutor::ReportStatusCallback()));
> Why leave this as a comment? I'd imagine it will be called for the coord fr
added todo, but i don't want to remove it, just to make sure we don't forget.

PS9, Line 645: s before optimizing the LLVM module. The other expr
> this and other cases where we index into fragment_instance_states_ would be

Line 680:          fragment_idx < request.fragments.size(); ++fragment_idx) {
> comment where they are created.

PS9, Line 711: 
> why cref(params)? Won't the reference (not the value) be copied by bind(), 
without the cref it's trying to copy the parameters and then complains about a 
missing copy c'tor (i'm glad there wasn't one).

PS9, Line 712: 
> several of these can be omitted if schedule_ is a member variable. Again, d

PS9, Line 735: for (const MtFragmentExecParams& fragment_params: s
> why is this a comment?

PS9, Line 746: gOptions* instance
> same thing about using state_idx_

PS9, Line 1453: u
> please make sure to run git-clang-format over this patch when you commit it

PS9, Line 1555: SetExecPlanFragmentParams(exec_pa
> It'd be nice to avoid passing these through the fn. I don't see any reason 
these are shared among all instances to which they apply, so i'd rather not 
create copies everywhere.

they're also only used during instance startup, so i don't see why passing it 
as a parameter is inappropriate.

PS9, Line 1564:       rpc_params.fragment_instance_ctx.per_node_scan_ranges);
              :   VLOG_FILE << "making rpc: ExecPlanFragment"
> same thing about the state_idx_ parameter

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

Line 101: class Coordinator {
> nit: extra line
File be/src/runtime/coordinator.h:

PS9, Line 547: e.
             :   void MtStartRemoteFInstances();
> Looks to me like it sets up the state for all non-coordinator fragment inst
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?
i was going to.
File be/src/scheduling/query-schedule.h:

Line 184:   const TPlanFragment& GetContainingFragment(PlanNodeId node_id) 
const {
> What's 'it' - the QuerySchedule? It doesn't get created in the scheduler, b
i'm not sure i find that particularly problematic, but either way this doesn't 
need to get addressed as part of this change.

PS3, Line 196: d::vector<FragmentExecParams>* exec_params() { return 
&fragment_exec_params_; }
             :   const std::vector<FragmentExecParams>& exec_params() const {
             :     return fragment_exec_params_;
> Done
the dcheck provides additional guarantees.
File be/src/scheduling/

PS4, Line 541:   // We adjust all replicas with memory distance less than 
base_distance to base_distance
             :   // and collect all replicas with equal or better distance 
> i'm not sure what to add.
added an explanation
File be/src/scheduling/

PS9, Line 942: int64_t assigned_bytes = 0;
> any reason you can't do this now?
it's purely additive, and there's lots of functionality that's still missing 
(joins, for instance) and will need to get implemented at a later date.
File be/src/scheduling/simple-scheduler.h:

PS4, Line 499: 
> QuerySchedule is really just a helper class which functions as a container 
added to class comment
File be/src/service/

PS9, Line 148: VLOG_FILE 
> This might be noisy for QUERY

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I240445199e22f009f4e72fdb8754eb8d77e3d680
Gerrit-PatchSet: 10
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