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: (6 comments) This should be a big improvement. http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/exec/external-data-source-executor.cc File be/src/exec/external-data-source-executor.cc: http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/exec/external-data-source-executor.cc@79 PS1, Line 79: num_class_cache_hits_ = metrics->AddCounter( Thanks for doing this cleanup. 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@128 PS1, Line 128: /// - 'property' which is a value store which can be read and written only Maybe mention that the metric kinds can serve as a hint for how management software should present the metrics? I.e. it's not purely an internal concept. http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@144 PS1, Line 144: virtual T value() = 0; I feel like we should rename to GetValue()/SetValue() since they're no longer just plain inline functions (although I guess that wasn't true even before because of the CalculateValue() bit). If we make set_value() non-virtual like I suggest below, this comment may not apply there. 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; Does set_value() need to be a virtual function? I can't think of a case where we'd call set_value() without knowing the subclass of the metric and I don't think every metric subclass necessarily needs an implementation of this, since some metrics are computed on demand. http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@225 PS1, Line 225: virtual int64_t value() override { return CalculateValue(); } The combination of value_/value()/CalculateValue() seems a bit too complicated. Couldn't subclasses just override value() instead of having the additional CalculateValue() indirection. It's still a bit weird if a metric overrides CalculateValue() and never uses value_ but it seems like avoiding that would require refactoring the class hierarchy somehow, which probably isn't worth it. 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", It seems like this should still be exposed as a gauge since it's a numeric value that can go up and down. Some management tools use this kind of metadata to decide how to display metrics. I think the current SimpleProperty implementation is fine for this use case but we should allow it to be labelled as a GAUGE for external purposes. -- 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: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 12 Jan 2018 16:41:48 +0000 Gerrit-HasComments: Yes
