impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3823: Add timer to measure Parquet footer reads
Date Tue, 18 Oct 2016 23:22:07 GMT
Sailesh Mukil has posted comments on this change.

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


Patch Set 8:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/4371/8/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

PS8, Line 204: Contrary to 
> "Unlike"
Done


PS8, Line 204: a 
> remove
Done


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 
Yes, I've changed it to say value() instead.


Line 209:   SummaryStatsCounter(TUnit::type unit, int32_t total_num_values,
> Comment what this constructor is used for (is it when merging two SSCounter
It's for replacing the existing counter with new values.


PS8, Line 216: total_num_values
> can total_num_values be 0?
I don't think it happens in practice, but I've made a change to handle that case too.


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


PS8, Line 255: SpinLock counter_lock_;
> any reason not to call this lock_ ?
Nope, changed it.


http://gerrit.cloudera.org:8080/#/c/4371/8/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

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


PS8, Line 669: are
> is
Done


PS8, Line 1019: { min_ = new_value; }
> remove parentheses for single-line if statements.
Done


http://gerrit.cloudera.org:8080/#/c/4371/8/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

PS8, Line 317: \
> remove
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message