Henry Robinson has posted comments on this change. Change subject: IMPALA-4314: Standardize on MT-related data structures ......................................................................
Patch Set 2: (12 comments) Mostly trivial comments, next round should be +2 from me. Great to see all the deleted code! 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? 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 fragment or not. PS2, Line 77: if (coord_fragment.partition.type == TPartitionType::UNPARTITIONED) { Let's check stmt_type == QUERY. This check is indirect and prone to future bugs (what if we introduce some other kind of sink that always has UNPARTITIONED input?). Line 142: map<TPlanNodeId, int>& node_map = host_it->second; const? PS2, Line 162: ) { long line Line 244: if (fragment->partition.type == TPartitionType::UNPARTITIONED) { > but this should be callable for a non-query stmt (so the coordinator doesn' But in that case, there is no coordinator fragment (if it's not a QUERY). Just to be clear, my understanding is that there is a coord fragment iff the query produces result sets, i.e. it has a PlanRootSink and is of type QUERY. 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 accidental nullptr mistakes. PS2, Line 113: Verifies that the schedule is well-formed: : /// - all fragments have a FragmentExecParams : /// - all scan ranges are assigned What's the behaviour if it's not valid? Presumably DCHECK, but should mention that here. PS2, Line 116: Verify Validate()? AssertWellFormed()? PS2, Line 203: RuntimeProfile::EventSequence* query_events_; This is here so that the schedule can mark events on the query-wide event timeline. 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). http://gerrit.cloudera.org:8080/#/c/4853/2/be/src/scheduling/simple-scheduler.h File be/src/scheduling/simple-scheduler.h: PS2, Line 437: /// For each instance of fragment_params's input fragment, create a collocated : /// instance for fragment_params's fragment. I can't quite parse this. -- 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
