Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-4187: Switch RPC latency metrics to histograms
Patch Set 2:
PS2, Line 179: int32_t
PS2, Line 181: 3
nit: I've previously learned that it's a good practice not to use "magic"
constants such as used here, or if it's absolutely necessary, a small comment
should explain what the use of said constant is.
Is that something that's worth following?
There's no shared state access before this line. Worth locking from here?
And releasing the lock here?
PS2, Line 70: histogram_->Increment(val);
On an unrelated note, I looked through the implementation of HdrHistogram and
there seems to be use of a lot of atomic operations, but that doesn't make it
race-free (which is why it makes sense to use a lock here).
Any idea why HdrHistogram is written that way? (Not super important. I was just
curious; if you're not sure, we can always revisit it later)
PS2, Line 107: boost::scoped_ptr<HdrHistogram> histogram_;
I know we had a scoped_ptr vs unique_ptr discussion thread recently, but it
doesn't seem like we reached a verdict.
It doesn't seem like we intend to move() or release() this pointer, but are we
okay with increasing dependency on boost by adding new scoped_ptrs?
To view, visit http://gerrit.cloudera.org:8080/4516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <j...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>