Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9012 )
Change subject: IMPALA-2397: Use atomics for metrics ...................................................................... Patch Set 2: (10 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@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 Done http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@144 PS1, Line 144: /// Returns the current value. Thread-safe. > I feel like we should rename to GetValue()/SetValue() since they're no long Renamed to GetValue() / SetValue(). Keep them as pure virtual as their implementations can be different in derived classes but they share the implementation for the ToJson() interface. http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@147 PS1, Line 147: /// Sets the current value. Thread-safe. > Does set_value() need to be a virtual function? I can't think of a case whe Currently, there are only two derived classes for SimpleMetric and both of them implement this interface albeit differently. So, it seems to make sense to have it as virtual for now until we have more classes which inherit SimpleMetric but somehow doesn't implement SetValue(). http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@215 PS1, Line 215: operty; > nit: maybe rename to metric_kind_t to make it more clear that it's a type? Done http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@225 PS1, Line 225: } > The combination of value_/value()/CalculateValue() seems a bit too complica Good idea. Renamed value() to GetValue(). http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@232 PS1, Line 232: /// need to take care of synchronization among sources. > Could this be a compile time check / assertion? I believe the compiler will most likely fold this as a constant during compilation so it's a no-op for GAUGE. http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@252 PS1, Line 252: typedef class AtomicMetric<TMetricKind::COUNTER> IntCounter; > I think it might be slightly easier to follow if we move these below the cl Done http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@265 PS1, Line 265: int64_t sum = 0; > The base class comment says that this happens atomically, but this implemen The new code and the old code both don't hold locks of all gauges to prevent them from being written before reading them. So, although each read of the gauges is atomic by itself, there is no guarantee on atomicity between all the gauges. The base class comments only means that it's up to the derived class to implement its own synchronization and the model above apparently seems good enough for SumGauge. Note that value() and set_value() are racy by nature as the value can change right after the value is read. The atomicity guarantee has more to do with read-modify-write between multiple writers and atomic read of the value (i.e. no half written 64-bit value). http://gerrit.cloudera.org:8080/#/c/9012/1/be/src/util/metrics.h@284 PS1, Line 284: > nit: this might fit into a single line Done 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 Unfortunately, if we set this to a GAUGE, we need to back it with a gauge in the code too. In which case, we may need to implement DoubleGauge. -- 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: 2 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 17 Jan 2018 19:18:31 +0000 Gerrit-HasComments: Yes
