You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Chris Riccomini <cr...@apache.org> on 2013/09/11 03:13:34 UTC
Re: Review Request 13912: SAMZA-28
-----------------------------------------------------------
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
Re: Review Request 13912: SAMZA-28
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13912/
-----------------------------------------------------------
(Updated Oct. 4, 2013, 9:42 p.m.)
Review request for samza.
Repository: samza
Description (updated)
-------
adding a buffer gauge for blocking envelope map, so we can inject buffer sizes dynamically on request, rather than updating them constantly.
Merge remote-tracking branch 'origin/master' into SAMZA-28
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