Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12956 )
Change subject: IMPALA-8375: Add metrics for spill disk usage ...................................................................... Patch Set 4: (4 comments) Looks good, just had a few comments about comments and naming. http://gerrit.cloudera.org:8080/#/c/12956/4/be/src/runtime/tmp-file-mgr.h File be/src/runtime/tmp-file-mgr.h: http://gerrit.cloudera.org:8080/#/c/12956/4/be/src/runtime/tmp-file-mgr.h@429 PS4, Line 429: scratch_space_bytes_used_high_water_mark_metric_ This could be more concise, e.g. scratch_bytes_used_metric_ http://gerrit.cloudera.org:8080/#/c/12956/4/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/12956/4/be/src/util/metrics.h@254 PS4, Line 254: /// maintains the current value. Note that since two separate atomics are used Everything except the first sentence is describing implementation details. This is fine, but it's helpful to separate from the actual interface. Maybe just have as a separate implementation notes section, e.g. /// ... description of public interface... /// /// Implementation notes: /// ... http://gerrit.cloudera.org:8080/#/c/12956/4/be/src/util/metrics.h@271 PS4, Line 271: Atomicaly Atomically. http://gerrit.cloudera.org:8080/#/c/12956/4/be/src/util/metrics.h@271 PS4, Line 271: hwm_value_ Prefer not to mention private members in public interface. Could just rewrite as: /// Returns the current high water mark value. -- To view, visit http://gerrit.cloudera.org:8080/12956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia1b3dd604c7234a8d8af34d70ca731544a46d298 Gerrit-Change-Number: 12956 Gerrit-PatchSet: 4 Gerrit-Owner: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 16 Apr 2019 05:44:38 +0000 Gerrit-HasComments: Yes
