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:

(26 comments)

regarding testing: all tests passed

http://gerrit.cloudera.org:8080/#/c/4054/10/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS10, Line 294: boost
> Is there any reason for the boost sets?
a complete overhaul of coordinator.cc 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
Done


Line 1975:           
fragment_profiles_[instance_state.fragment_id()].num_instances);
> in the else case you can DCHECK that exec_summary.exec_stats is the right s
Done


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


PS10, Line 2253: 
               : namespace {
               : 
               : // Make a PublishFilter rpc to 'impalad' for given 
fragment_instan
> nit wrap to 90chars
Done


http://gerrit.cloudera.org:8080/#/c/4054/10/be/src/runtime/coordinator.h
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_
Done


http://gerrit.cloudera.org:8080/#/c/4054/10/be/src/runtime/runtime-filter-bank.cc
File be/src/runtime/runtime-filter-bank.cc:

PS10, Line 84: QUERY
> will this (and below) be noisy?
this happens once for each filter. vlog_file seems too high a log level.


http://gerrit.cloudera.org:8080/#/c/4054/10/be/src/scheduling/query-schedule.h
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
Done


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


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


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.
Done


PS10, Line 172:   const TPlanFragment* GetCoordFragment() const;
              : 
              :   /// Return all fragments belonging to exec request in 
'fragments'.
              :   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 
mt_fragment_exec_params_[GetFragmentId(node_id)].fragment;
> I think you can remove this- there's only 1 usage I can see in SimpleSchedu
Done


PS10, Line 210: 
              :   const FInstanceExecParams& GetCoordInstanceExecParams() const 
{
              :     const TPlanFragment& coord_fragment =  
request_.mt_plan_exec_info[0].fragments[0];
              :     DCHECK_EQ(coord_fragment.partition.type, 
TPartitionType::UNPARTITIONED);
              :    
> I only see this used by the fn below, and I only see the fn below used once
Done


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 
Done


http://gerrit.cloudera.org:8080/#/c/4054/10/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:

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


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 
Done


http://gerrit.cloudera.org:8080/#/c/4054/10/be/src/scheduling/simple-scheduler.cc
File be/src/scheduling/simple-scheduler.cc:

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.


http://gerrit.cloudera.org:8080/#/c/4054/10/common/thrift/ExecStats.thrift
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
Done


http://gerrit.cloudera.org:8080/#/c/4054/10/common/thrift/Planner.thrift
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.


http://gerrit.cloudera.org:8080/#/c/4054/10/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:

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
> TODO-MT
Done


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


-- 
To view, visit http://gerrit.cloudera.org:8080/4054
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I240445199e22f009f4e72fdb8754eb8d77e3d680
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <l...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to