Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for metrics ...................................................................... Patch Set 1: (2 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@147 PS1, Line 147: virtual void set_value(const T& value) = 0; > Currently, there are only two derived classes for SimpleMetric and both of What's the point of making it a virtual function if it doesn't need to be? It seems better to just have a regular function on the derived classes - it's more efficient and easier to understand which function is being called. I checked out this PS and made it non-virtual and it seems to compile and run fine. http://gerrit.cloudera.org:8080/#/c/9012/1/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/9012/1/common/thrift/metrics.json@886 PS1, Line 886: "kind": "PROPERTY", > Unfortunately, if we set this to a GAUGE, we need to back it with a gauge i I think SimpleProperty<double> implements all the functionality needed for this one case, it would just need to declare itself as a TMetricKind::GAUGE. I.e we could implement a skeleton DoubleGauge that only supports SetValue() and GetValue(). This isn't the end of the world, but it would be good if we could avoid the internal implementation details leaking out into our metadata about metrics. -- 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 <k...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 17 Jan 2018 20:14:32 +0000 Gerrit-HasComments: Yes
