Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201609250508.u8P58Nk5004238@ip-10-146-233-104.ec2.internal>
Date: Sun, 25 Sep 2016 05:08:23 +0000
From: "Henry Robinson (Code Review)" <gerrit@cloudera.org>
To: Marcel Kornacker <marcel@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Alex Behm <alex.behm@cloudera.com>, Dan Hecht <dhecht@cloudera.com>,
        Mostafa Mokhtar <mmokhtar@cloudera.com>, Lars Volker <lv@cloudera.com>,
        Matthew Jacobs <mj@cloudera.com>
Reply-To: henry@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3902=3A_Scheduler_improvements_for_running_multiple_fragment_instances_on_a_single_backend=0A?=
In-Reply-To: <gerrit.1471565970000.I240445199e22f009f4e72fdb8754eb8d77e3d680@gerrit.cloudera.org>
References: <gerrit.1471565970000.I240445199e22f009f4e72fdb8754eb8d77e3d680@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Sun, 25 Sep 2016 05:08:30 -0000

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 <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-HasComments: Yes