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