samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Riccomini" <criccom...@apache.org>
Subject Re: Review Request 13912: SAMZA-28
Date Wed, 11 Sep 2013 01:13:34 GMT

-----------------------------------------------------------
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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message