Henry Robinson has posted comments on this change.

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


Patch Set 14:

(15 comments)

I think it would be good to have Alex review Frontend.java again - he said it 
looked good overall in PS3 so hopefully there won't be many comments.

http://gerrit.cloudera.org:8080/#/c/4054/12/be/src/scheduling/query-resource-mgr.cc
File be/src/scheduling/query-resource-mgr.cc:

Line 1: // Licensed to the Apache Software Foundation (ASF) under one
Remove this file.


http://gerrit.cloudera.org:8080/#/c/4054/12/be/src/scheduling/query-resource-mgr.h
File be/src/scheduling/query-resource-mgr.h:

Line 1: // Licensed to the Apache Software Foundation (ASF) under one
Did your rebase go wrong? This file should be removed.


http://gerrit.cloudera.org:8080/#/c/4054/12/be/src/scheduling/query-schedule.cc
File be/src/scheduling/query-schedule.cc:

PS12, Line 68: if (plan_node_to_fragment_idx_.size() < node.node_id + 1) {
             :             plan_node_to_fragment_idx_.resize(node.node_id + 1);
             :             plan_node_to_plan_node_idx_.resize(node.node_id + 1);
             :           }
I think you can find the max node ID in InitMtFragmentExecParams if you also 
loop over the plan nodes there. That seems better than all this conditional 
resizing, which is confusing to read.


PS12, Line 105: std
remove std::


http://gerrit.cloudera.org:8080/#/c/4054/12/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:

PS12, Line 184: Returns next instance id. Instance ids are consecutive numbers 
generated from
              :   /// the query id.
shouldn't that be the opposite? Start at 0 if there's a coordinator fragment.

Might be better reworded as "The coordinator fragment instance, if there is 
one, has ID 0, all other fragment instances start from 1."


PS12, Line 278: 
what's 'static info'?


http://gerrit.cloudera.org:8080/#/c/4054/14/be/src/service/fragment-mgr.cc
File be/src/service/fragment-mgr.cc:

PS14, Line 23: #include <iomanip>
not needed (see below), remove


PS14, Line 42: << hex << exec_params.fragment_instance_ctx.fragment_instance_id 
<< dec
use PrintId().


http://gerrit.cloudera.org:8080/#/c/4054/14/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 848:   random_generator uuid_generator;
remove this line


PS14, Line 1080: << hex << params.fragment_instance_id
Use PrintId() here instead of hex / dec.


http://gerrit.cloudera.org:8080/#/c/4054/14/be/src/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:

Line 443:   Status status = exec_env_->scheduler()->Schedule(schedule_.get());
Leave a TODO to simplify this as discussed.


http://gerrit.cloudera.org:8080/#/c/4054/14/be/src/service/query-options.cc
File be/src/service/query-options.cc:

Line 398:       case TImpalaQueryOptions::MT_DOP: {
When the patch that added MT_NUM_CORES was committed, I asked to consider a 
mechanism for hiding query options. DId that come to anything? It's unfortunate 
that we're going to commit a query option that (AFAICT) will break clients if 
it's turned on.


http://gerrit.cloudera.org:8080/#/c/4054/14/be/src/util/container-util.h
File be/src/util/container-util.h:

PS14, Line 79: V* FindOrInsert(std::unordered_map<K,V>* m, const K& key, const 
V& default_val) {
             :   typename std::unordered_map<K,V>::iterator it = m->find(key);
             :   if (it == m->end()) {
             :     it = m->insert(std::make_pair(key, default_val)).first;
             :   }
             :   return &it->second;
             : }
             : 
             : template <typename K, typename V>
             : V* FindOrInsert(boost::unordered_map<K,V>* m, const K& key, 
const V& default_val) {
             :   typename boost::unordered_map<K,V>::iterator it = m->find(key);
             :   if (it == m->end()) {
             :     it = m->insert(std::make_pair(key, default_val)).first;
             :   }
             :   return &it->second;
             : }
Just add the map type as a template parameter and remove the other versions of 
this.


http://gerrit.cloudera.org:8080/#/c/4054/14/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

PS14, Line 185: backend
so this is independent of the number of fragment instances for that query on 
that backend?


PS14, Line 333: nd the id of the first fragment instance (the coordinator 
instance)
              :   // are identical.
isn't this true now only if there is a coordinator fragment? Reword to make 
that clearer - looks like first fragment instance is always 0 regardless of 
whether there is coord fragment.


-- 
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: 14
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: Internal Jenkins
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