Sailesh Mukil has posted comments on this change.

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

Patch Set 6:

File be/src/exec/

Line 192:   Status footer_status;
> why was this moved to a separate line?
Sorry, I previously introduced a code block with "{ }" and moved this so that 
it wouldn't go out of scope. But I've since removed it, so it's no longer 
needed. Done.
File be/src/util/runtime-profile-counters.h:

PS6, Line 49: TIMER
> is there anything time-specific about this counter?  why not ADD_SUMMARY_ST
It's the same reason as why we have ADD_COUNTER() and ADD_TIMER(); they both 
are the same, but ADD_TIMER() uses the default unit as TUnit::TIME_NS.

PS6, Line 205: AveragedCounter
> do we plan to rename that? would be nice.
It technically is an AveragedCounter, so I'm out of ideas for a better name. Do 
you have any suggestions?

PS6, Line 206: untimeProfile::Counter
> what does the base class' value_ hold?
It holds the average. I've added that in a comment now.

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 unlo
Since it's only a read, I figured even if the CPU reorders the read instruction 
for that variable's address to happen a few cycles earlier than intended, it 
doesn't matter, as how is it any different than having an atomic 'min_.Load()' 
where the value of 'min_' changes just after the Load() returns with a value.

What do you think?

PS6, Line 240: This function 
> delete
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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf
Gerrit-PatchSet: 6
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

Reply via email to