impala-reviews mailing list archives

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

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


Patch Set 6:

(8 comments)

Do we have any counters where we track running sum and number of samples and compute the average
by hand (or by the user)? i.e. any other existing places we should use this counter?

http://gerrit.cloudera.org:8080/#/c/4371/6/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:

Line 192:   Status footer_status;
why was this moved to a separate line?


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

PS6, Line 49: TIMER
is there anything time-specific about this counter?  why not ADD_SUMMARY_STATS_COUNTER(profile,
name, unit)?


PS6, Line 205: AveragedCounter
do we plan to rename that? would be nice.


PS6, Line 206: untimeProfile::Counter
what does the base class' value_ hold?


PS6, Line 221: INT64_MAX
we usually use numeric_limits


Line 228:   int32_t total_num_values() const { return total_num_values_; }
what's the thread safety guarantees of our counters?  why can these be unlocked/non-atomic?


PS6, Line 240: This function 
delete


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

PS6, Line 159: min/max average
this sounds like a "min-average" "max-average" or something. how about just: Adds a counter
that tracks min, max and average ... 
or something like that.


-- 
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message