impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3902: Scheduler improvements for running multiple fragment instances on a single backend
Date Sun, 25 Sep 2016 18:39:48 GMT
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 14:

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


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


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


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 fragmen
clarified


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


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
Done


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


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
Done


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


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


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
that exceeds my template craft. i'll leave a todo.


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 o
yes


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

Mime
View raw message