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 <sail...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
