Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for metrics ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@215 PS1, Line 215: metric_kind nit: maybe rename to metric_kind_t to make it more clear that it's a type? http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@232 PS1, Line 232: DCHECK(metric_kind != TMetricKind::COUNTER || delta >= 0) Could this be a compile time check / assertion? http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@252 PS1, Line 252: typedef class SimpleProperty<bool> BooleanProperty; I think it might be slightly easier to follow if we move these below the class declaration of SimpleProperty. http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@265 PS1, Line 265: virtual int64_t CalculateValue() const override { The base class comment says that this happens atomically, but this implementation looks like it doesn't. Is that intended? http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@284 PS1, Line 284: virtual int64_t CalculateValue() const override { nit: this might fit into a single line -- To view, visit http://gerrit.cloudera.org:8080/9012 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48dfa5443cd771916b53541a0ffeaf1bcc7e7606 Gerrit-Change-Number: 9012 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 12 Jan 2018 17:47:42 +0000 Gerrit-HasComments: Yes
