impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3823: Add timer to measure Parquet footer reads
Date Mon, 17 Oct 2016 21:05:38 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3823: Add timer to measure Parquet footer reads

Patch Set 8: Code-Review+1

File be/src/util/runtime-profile-counters.h:

PS8, Line 204: Contrary to 

PS8, Line 204: a 

PS8, Line 206: The average is stored in the 'value_' member of the base class.
Referring to member variables in a class description is usually a sign the text is leaking
implementation details. Do you mean to say that value() returns the average?

Line 209:   SummaryStatsCounter(TUnit::type unit, int32_t total_num_values,
Comment what this constructor is used for (is it when merging two SSCounters together?)

PS8, Line 216: total_num_values
can total_num_values be 0?

PS8, Line 247: This keeps track of t
Remove - just "The total..." is sufficient.

PS8, Line 255: SpinLock counter_lock_;
any reason not to call this lock_ ?
File be/src/util/

PS8, Line 664: /* Print all SummaryStatsCounters as following:
             :     <Name>: (Avg: <value> ; Min: <min_value> ; Max: <max_value>
             :     Number of samples: <total>) */
Comment style?

PS8, Line 669: are

PS8, Line 1019: { min_ = new_value; }
remove parentheses for single-line if statements.
File tests/query_test/

PS8, Line 317: \

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message