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


regarding testing: all tests passed
File be/src/runtime/

PS10, Line 294: boost
> Is there any reason for the boost sets?
a complete overhaul of is out of scope.

i have tried to switch to std:: and away from boost:: elsewhere, and it often 
requires very sweeping renaming, which i don't want to attempt here

PS10, Line 750: std::cref(insta
> why doesn't this params need a cref, but the one in StartRemoteFragments() 
it probably called the default copy c'tor. fixed.

PS10, Line 1971:  TODO-MT: remove
> please mention this only happens for ST because in the MT case it is set up

Line 1975:           
> in the else case you can DCHECK that exec_summary.exec_stats is the right s

PS10, Line 2083: rorLogMap merged;
> After the coord is treated as a remote FragmentInstanceState?

PS10, Line 2253: 
               : namespace {
               : // Make a PublishFilter rpc to 'impalad' for given 
> nit wrap to 90chars
File be/src/runtime/coordinator.h:

PS10, Line 259:   /// 1. lock_
              :   /// 2. FragmentInstanceState::lock_
              :   boost::mutex
> this seems to contradict the wording in FragmentInstanceState::lock_
File be/src/runtime/

PS10, Line 84: QUERY
> will this (and below) be noisy?
this happens once for each filter. vlog_file seems too high a log level.
File be/src/scheduling/query-schedule.h:

PS10, Line 90:  shared across fragment instances
> for all fragment instances
not sure what you mean.

Line 106:   std::vector<FInstanceExecParams> instance_exec_params;
> remove extra line

Line 153: 
> when coordinator is handled as a remote fragment?

Line 157:   /// (in effect the number of remote instances)
> when coordinator is handled as a remote fragment?

PS10, Line 143:   /// The following 4 functions need to be replaced once we 
stop special-casing
              :   /// the coordinator instance in the coordinator.
              :   /// The replacement is a single function int 
GetNumFInstances() (which includes
              :   /// the coordinator instance).
              :   /// TODO-MT: remove; this is actually only the number of 
remote instances
              :   /// (from the coordinator's perspective)
              :   void set_num_fragment_instances(int64_t 
num_fragment_instances) {
              :     num_fragment_instances_ = num_fragment_instances;
              :   }
              :   /// Returns the number of fragment instances registered with 
this schedule.
              :   /// MT: total number of fragment instances
              :   /// ST: value set with set_num_fragment_instances(); excludes 
coord instance
              :   /// (in effect the number of remote instances)
              :   /// TODO-MT: get rid of special-casing of coordinator 
instance and always return the
              :   /// total
              :   int GetNumFragmentInstances() const;
              :   /// Returns the total number of fra
> This is all still pretty confusing.

PS10, Line 172:   const TPlanFragment* GetCoordFragment() const;
              :   /// Return all fragments belonging to exec request in 
              :   void GetTPlanFragments(std::vector<const TPlanFragment*>* 
fragments) const;
> this is very confusing. I think you can get rid of the first one if you cha
you can't use GetContainingFragment() there, because it references 
mt_fragment_exec_params_, but GetFragmentId() works.

PS10, Line 187: 
              :   const TPlanFragment& GetContainingFragment(PlanNodeId 
node_id) const {
              :     return 
> I think you can remove this- there's only 1 usage I can see in SimpleSchedu

PS10, Line 210: 
              :   const FInstanceExecParams& GetCoordInstanceExecParams() const 
              :     const TPlanFragment& coord_fragment =  
              :     DCHECK_EQ(coord_fragment.partition.type, 
> I only see this used by the fn below, and I only see the fn below used once

PS10, Line 244: 
              :   /// Maps from plan node id to its index in plan.nodes. Filled 
in c'tor.
              :   std::vector<int32_t> plan_node_to_plan_node_idx_
> I think we can get rid of this by changing ComputeScanRangeAssignment, and 
File be/src/scheduling/scheduler.h:

PS10, Line 49:  and assigns fragments to hosts based on scan
> remove

PS10, Line 51:   /// returning.
             :   virtual Status Schedule(QuerySchedule* schedule) = 0;
> whoops looks like we missed this in Henry's change. Can you change this to 
File be/src/scheduling/

PS10, Line 900:  // TODO-MT: call AdmitQuery()
> just to follow up on our conversation in person: 
i'm not saying we shouldn't do it at all, but there is no need to do it as part 
of this change. there are plenty of things that still don't work in mt mode, 
and this is just one of them. those will all get addressed in future changes.
File common/thrift/ExecStats.thrift:

PS10, Line 69: 
             :   // If true, this plan node is an exchange node that is the 
receiver of a broadcast.
             :   8: optional bool is_broadcast
             : }
> I don't see it used anywhere. I tested removing it and everything still com
File common/thrift/Planner.thrift:

PS10, Line 35:   // TODO: should this be called idx, to distinguish more 
clearly from a
             :   // globally unique id?
> I'd prefer to call this an idx given that we have fragment_instance_id of t
let's think about this some more tomorrow.

PS10, Line 37: TFragmentId
> ... and then maybe this would be better as an int? I'm not sure the typedef
i think the typedef makes it a bit easier to distinguish from all the other 
ints that we have flying around.
File fe/src/main/java/com/cloudera/impala/service/

PS10, Line 914:    */
              :   private TPlanExecInfo createPlanExecInfo(PlanFragment 
planRoot, Planner planner,
> We've already accepted a split between MT & ST paths, so I think this condi
actually, in many cases the current st paths can be removed and we can switch 
to the mt paths and data structures (while still generating the current st 
plans, but inside the mt-expanded data structure).

i'll add that to the commit msg.

PS10, Line 982: u

PS10, Line 1029:     LOG.debug("cre
> remove

To view, visit
To unsubscribe, visit

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