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-4314: Standardize on MT-related data structures
Date Thu, 27 Oct 2016 22:29:05 GMT
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 <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message