----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13912/ -----------------------------------------------------------
(Updated Sept. 11, 2013, 1:13 a.m.) Review request for samza. Changes ------- So, I think the options we have are: 1) Synchronize the Gauge, as we're doing now. 2) Write some kind of funky CounterGauge that is a Gauge that has inc/dec. 3) Use the AtomicReference inside Gauge to atomically update the value in the Gauge. 4) Use a Gauge<AtomicInteger>, and inc/dec/set the Atomic integer. 5) Ignore the synchronization problem. (1) is annoying because it requires locking on the Gauge. This doesn't impact perf, according to my tests (single partition read/write). (2) is hacky. It would also involve changing the MetricsRegistry to make the Gauge injectable. Right now, newGauge creates the Gauge internally. This leads to more hackiness, where we have to implement a CounterGauge that extends Gauge<Integer>, and overrides all methods (inc, dec, set) to atomically increment the value. (3) seems better than 1 and provides the same guarantees, but it means we could be re-trying the read/modify/write operation multiple times in cases where a single partition is being processed modified repeatedly. (4) would require metrics reporters to support atomic long/atomic integer as numbers, rather than strings. It's also slightly hacky and bizarre to implement an AtomicReference that points to an AtomicInteger. (5) could potentially lead to cases where the measured count for a buffer is wrong for a long period of time. Of these, I think 1, 3, and 4 are the most acceptable. I chose to use implement (4) because it isn't hacky, is lock-free, and uses the atomic reference in the Gauge as it's meant to be used. I ran the perf test and observed no measurable performance degradation. Didn't specifically check for thrashing on compreAndSet, but I believe significant thrashing would manifest itself as poor performance. Repository: samza Description (updated) ------- reverse change to common and counters. reverse change to common and counters. enabling metrics for blocking envelope map doing non-locking metrics update in blocking envelope map. lock gauges when we update, since this is not atomic, and there's a race condition between poll and add switch counter to gauge for buffered message count map Diffs (updated) ----- samza-api/src/main/java/org/apache/samza/metrics/Gauge.java 14db2d367c151e9c3d435db29888ab64f74c1d3a samza-api/src/main/java/org/apache/samza/util/BlockingEnvelopeMap.java c1dd27988f130f38adcd39e0bc811b6f8074ca6b Diff: https://reviews.apache.org/r/13912/diff/ Testing ------- Thanks, Chris Riccomini
