From 474bbf31937e748fab620a9293b3b9bcbbcd3df4 Mon Sep 17 00:00:00 2001 From: Geir Magnusson Jr Date: Tue, 9 Jun 2009 23:00:47 -0400 Subject: [PATCH 1/2] RequestCounter : fixed bug where infinite loop was possible (and happened) Under heavy load, it was possible to spin a thread forever if the accum was changed during the calculation of the time expiry for the bucket. I think this new impl has no chance of failure or loss. --- .../voldemort/store/stats/RequestCounter.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/java/voldemort/store/stats/RequestCounter.java b/src/java/voldemort/store/stats/RequestCounter.java index 09dc2745a0..b012e98ddc 100644 --- a/src/java/voldemort/store/stats/RequestCounter.java +++ b/src/java/voldemort/store/stats/RequestCounter.java @@ -60,18 +60,23 @@ public int getDuration() { } private Accumulator getValidAccumulator() { + Accumulator accum = values.get(); long now = System.currentTimeMillis(); + if(now - accum.startTimeMS > durationMS) { Accumulator newWithTotal = accum.newWithTotal(); - while(true) { - if(values.compareAndSet(accum, newWithTotal)) { - return newWithTotal; - } + values.set(newWithTotal); + + /* + * try to set. if we fail, then someone else set it, so just keep going + */ + if(values.compareAndSet(accum, newWithTotal)) { + return newWithTotal; } - } else { - return accum; } + + return values.get(); } /* From 679e1d5cd007a0a9cb2813bd155622d7a1e904bd Mon Sep 17 00:00:00 2001 From: Geir Magnusson Jr Date: Wed, 10 Jun 2009 04:32:43 -0400 Subject: [PATCH 2/2] RequestCounter : remove my spurious set() (thanks Ismael) and refactor to be more linear --- .../voldemort/store/stats/RequestCounter.java | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/java/voldemort/store/stats/RequestCounter.java b/src/java/voldemort/store/stats/RequestCounter.java index b012e98ddc..c6be603e86 100644 --- a/src/java/voldemort/store/stats/RequestCounter.java +++ b/src/java/voldemort/store/stats/RequestCounter.java @@ -64,16 +64,21 @@ private Accumulator getValidAccumulator() { Accumulator accum = values.get(); long now = System.currentTimeMillis(); - if(now - accum.startTimeMS > durationMS) { - Accumulator newWithTotal = accum.newWithTotal(); - values.set(newWithTotal); - - /* - * try to set. if we fail, then someone else set it, so just keep going - */ - if(values.compareAndSet(accum, newWithTotal)) { - return newWithTotal; - } + /* + * if still in the window, just return it + */ + if(now - accum.startTimeMS <= durationMS) { + return accum; + } + + /* + * try to set. if we fail, then someone else set it, so just return that new one + */ + + Accumulator newWithTotal = accum.newWithTotal(); + + if(values.compareAndSet(accum, newWithTotal)) { + return newWithTotal; } return values.get();