impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
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:

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
File be/src/scheduling/

PS2, Line 76: coord
> Nit: call this root_fragment. At this point we don't know if it's a coord f

PS2, Line 77: if (coord_fragment.partition.type == TPartitionType::UNPARTITIONED) {
> Let's check stmt_type == QUERY. This check is indirect and prone to future 

Line 142:       map<TPlanNodeId, int>& node_map = host_it->second;
> const?
l148 inserts

PS2, Line 162: ) {
> long line

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

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.
File be/src/scheduling/

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I465d0e15e2cf17cafe4c747d34c8f595d3645151
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-HasComments: Yes

View raw message