Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4314: Standardize on MT-related data structures ......................................................................
Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/4853/2/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: PS2, Line 485: /// Initializes the structures in fragment_profiles_. Must be : /// called before RPCs to start remote fragments. > Nit: this can be wrapped more concisely. Run clang-format over your changes Done http://gerrit.cloudera.org:8080/#/c/4853/2/be/src/scheduling/query-schedule.cc File be/src/scheduling/query-schedule.cc: PS2, Line 76: coord > Nit: call this root_fragment. At this point we don't know if it's a coord f Done PS2, Line 77: if (coord_fragment.partition.type == TPartitionType::UNPARTITIONED) { > Let's check stmt_type == QUERY. This check is indirect and prone to future Done Line 142: map<TPlanNodeId, int>& node_map = host_it->second; > const? l148 inserts PS2, Line 162: ) { > long line Done Line 244: if (fragment->partition.type == TPartitionType::UNPARTITIONED) { > But in that case, there is no coordinator fragment (if it's not a QUERY). changed to dcheck http://gerrit.cloudera.org:8080/#/c/4853/2/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: PS2, Line 95: // extract instance indices from instance_exec_params.instance_id : void > why not return vector<int>? Compiler will do RVO, and no chance for acciden Done PS2, Line 116: Verify > Validate()? AssertWellFormed()? change to validate PS2, Line 203: RuntimeProfile::EventSequence* query_events_; > This is here so that the schedule can mark events on the query-wide event t yeah, just a bit odd that it's part of the schedule. http://gerrit.cloudera.org:8080/#/c/4853/2/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: PS2, Line 489: // fake load-balancing for Kudu and Hbase: every split has length 1 : // TODO: implement more accurate logic for Kudu and Hbase > move this to line 496 (I thought it applied to the whole loop at first). Done -- To view, visit http://gerrit.cloudera.org:8080/4853 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I465d0e15e2cf17cafe4c747d34c8f595d3645151 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
