Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only allow bucket configuration to take effect on HystrixRollingNumber creation #825

Merged
merged 1 commit into from
Jun 21, 2015
Merged

Only allow bucket configuration to take effect on HystrixRollingNumber creation #825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions hystrix-core/src/jmh/java/com/netflix/hystrix/perf/RollingMaxPerfTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ public static class CounterState {

@Setup(Level.Iteration)
public void setUp() {
counter = new HystrixRollingNumber(
HystrixProperty.Factory.asProperty(100),
HystrixProperty.Factory.asProperty(10));
counter = new HystrixRollingNumber(100, 10);
}
}

Expand Down
4 changes: 1 addition & 3 deletions hystrix-core/src/jmh/java/com/netflix/hystrix/perf/RollingNumberPerfTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ public static class CounterState {

@Setup(Level.Iteration)
public void setUp() {
counter = new HystrixRollingNumber(
HystrixProperty.Factory.asProperty(100),
HystrixProperty.Factory.asProperty(10));
counter = new HystrixRollingNumber(100, 10);
}
}

Expand Down
2 changes: 1 addition & 1 deletion hystrix-core/src/main/java/com/netflix/hystrix/HystrixCollapserMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public static Collection<HystrixCollapserMetrics> getInstances() {
private final HystrixRollingPercentile percentileShardSize;

/* package */HystrixCollapserMetrics(HystrixCollapserKey key, HystrixCollapserProperties properties) {
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds(), properties.metricsRollingStatisticalWindowBuckets()));
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds().get(), properties.metricsRollingStatisticalWindowBuckets().get()));
this.key = key;
this.properties = properties;

Expand Down
2 changes: 1 addition & 1 deletion hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommandMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public static Collection<HystrixCommandMetrics> getInstances() {
private final HystrixEventNotifier eventNotifier;

/* package */HystrixCommandMetrics(HystrixCommandKey key, HystrixCommandGroupKey commandGroup, HystrixThreadPoolKey threadPoolKey, HystrixCommandProperties properties, HystrixEventNotifier eventNotifier) {
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds(), properties.metricsRollingStatisticalWindowBuckets()));
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds().get(), properties.metricsRollingStatisticalWindowBuckets().get()));
this.key = key;
this.group = commandGroup;
this.threadPoolKey = threadPoolKey;
Expand Down
2 changes: 1 addition & 1 deletion hystrix-core/src/main/java/com/netflix/hystrix/HystrixThreadPoolMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public static Collection<HystrixThreadPoolMetrics> getInstances() {
private final HystrixThreadPoolProperties properties;

private HystrixThreadPoolMetrics(HystrixThreadPoolKey threadPoolKey, ThreadPoolExecutor threadPool, HystrixThreadPoolProperties properties) {
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds(), properties.metricsRollingStatisticalWindowBuckets()));
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds().get(), properties.metricsRollingStatisticalWindowBuckets().get()));
this.threadPoolKey = threadPoolKey;
this.threadPool = threadPool;
this.properties = properties;
Expand Down
42 changes: 24 additions & 18 deletions hystrix-core/src/main/java/com/netflix/hystrix/util/HystrixRollingNumber.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,35 +52,41 @@ public class HystrixRollingNumber {

private static final Time ACTUAL_TIME = new ActualTime();
private final Time time;
final HystrixProperty<Integer> timeInMilliseconds;
final HystrixProperty<Integer> numberOfBuckets;
final int timeInMilliseconds;
final int numberOfBuckets;
final int bucketSizeInMillseconds;

final BucketCircularArray buckets;
private final CumulativeSum cumulativeSum = new CumulativeSum();

/**
* Construct a counter, with configurable properties for how many buckets, and how long of an interval to track
* @param timeInMilliseconds length of time to report metrics over
* @param numberOfBuckets number of buckets to use
*
* @deprecated Please use {@link HystrixRollingNumber(int, int) instead}. These values are no longer allowed to
* be updated at runtime.
*/
@Deprecated
public HystrixRollingNumber(HystrixProperty<Integer> timeInMilliseconds, HystrixProperty<Integer> numberOfBuckets) {
this(ACTUAL_TIME, timeInMilliseconds, numberOfBuckets);
this(timeInMilliseconds.get(), numberOfBuckets.get());
}

/* used for unit testing */
/* package for testing */HystrixRollingNumber(Time time, int timeInMilliseconds, int numberOfBuckets) {
this(time, HystrixProperty.Factory.asProperty(timeInMilliseconds), HystrixProperty.Factory.asProperty(numberOfBuckets));
public HystrixRollingNumber(int timeInMilliseconds, int numberOfBuckets) {
this(ACTUAL_TIME, timeInMilliseconds, numberOfBuckets);
}

private HystrixRollingNumber(Time time, HystrixProperty<Integer> timeInMilliseconds, HystrixProperty<Integer> numberOfBuckets) {
/* package for testing */ HystrixRollingNumber(Time time, int timeInMilliseconds, int numberOfBuckets) {
this.time = time;
this.timeInMilliseconds = timeInMilliseconds;
this.numberOfBuckets = numberOfBuckets;

if (timeInMilliseconds.get() % numberOfBuckets.get() != 0) {
if (timeInMilliseconds % numberOfBuckets != 0) {
throw new IllegalArgumentException("The timeInMilliseconds must divide equally into numberOfBuckets. For example 1000/10 is ok, 1000/11 is not.");
}
this.bucketSizeInMillseconds = timeInMilliseconds / numberOfBuckets;

buckets = new BucketCircularArray(numberOfBuckets.get());
}

/* package for testing */int getBucketSizeInMilliseconds() {
return timeInMilliseconds.get() / numberOfBuckets.get();
buckets = new BucketCircularArray(numberOfBuckets);
}

/**
Expand Down Expand Up @@ -258,7 +264,7 @@ public long getRollingMaxValue(HystrixRollingNumberEvent type) {
* NOTE: This is thread-safe because it's accessing 'buckets' which is a LinkedBlockingDeque
*/
Bucket currentBucket = buckets.peekLast();
if (currentBucket != null && currentTime < currentBucket.windowStart + getBucketSizeInMilliseconds()) {
if (currentBucket != null && currentTime < currentBucket.windowStart + this.bucketSizeInMillseconds) {
// if we're within the bucket 'window of time' return the current one
// NOTE: We do not worry if we are BEFORE the window in a weird case of where thread scheduling causes that to occur,
// we'll just use the latest as long as we're not AFTER the window
Expand Down Expand Up @@ -299,22 +305,22 @@ public long getRollingMaxValue(HystrixRollingNumberEvent type) {
} else {
// We go into a loop so that it will create as many buckets as needed to catch up to the current time
// as we want the buckets complete even if we don't have transactions during a period of time.
for (int i = 0; i < numberOfBuckets.get(); i++) {
for (int i = 0; i < numberOfBuckets; i++) {
// we have at least 1 bucket so retrieve it
Bucket lastBucket = buckets.peekLast();
if (currentTime < lastBucket.windowStart + getBucketSizeInMilliseconds()) {
if (currentTime < lastBucket.windowStart + this.bucketSizeInMillseconds) {
// if we're within the bucket 'window of time' return the current one
// NOTE: We do not worry if we are BEFORE the window in a weird case of where thread scheduling causes that to occur,
// we'll just use the latest as long as we're not AFTER the window
return lastBucket;
} else if (currentTime - (lastBucket.windowStart + getBucketSizeInMilliseconds()) > timeInMilliseconds.get()) {
} else if (currentTime - (lastBucket.windowStart + this.bucketSizeInMillseconds) > timeInMilliseconds) {
// the time passed is greater than the entire rolling counter so we want to clear it all and start from scratch
reset();
// recursively call getCurrentBucket which will create a new bucket and return it
return getCurrentBucket();
} else { // we're past the window so we need to create a new bucket
// create a new bucket and add it as the new 'last'
buckets.addLast(new Bucket(lastBucket.windowStart + getBucketSizeInMilliseconds()));
buckets.addLast(new Bucket(lastBucket.windowStart + this.bucketSizeInMillseconds));
// add the lastBucket values to the cumulativeSum
cumulativeSum.addBucket(lastBucket);
}
Expand Down
46 changes: 23 additions & 23 deletions hystrix-core/src/test/java/com/netflix/hystrix/util/HystrixRollingNumberTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,17 @@ public void testCreatesBuckets() {
try {
HystrixRollingNumber counter = new HystrixRollingNumber(time, 200, 10);
// confirm the initial settings
assertEquals(200, counter.timeInMilliseconds.get().intValue());
assertEquals(10, counter.numberOfBuckets.get().intValue());
assertEquals(20, counter.getBucketSizeInMilliseconds());
assertEquals(200, counter.timeInMilliseconds);
assertEquals(10, counter.numberOfBuckets);
assertEquals(20, counter.bucketSizeInMillseconds);

// we start out with 0 buckets in the queue
assertEquals(0, counter.buckets.size());

// add a success in each interval which should result in all 10 buckets being created with 1 success in each
for (int i = 0; i < counter.numberOfBuckets.get(); i++) {
for (int i = 0; i < counter.numberOfBuckets; i++) {
counter.increment(HystrixRollingNumberEvent.SUCCESS);
time.increment(counter.getBucketSizeInMilliseconds());
time.increment(counter.bucketSizeInMillseconds);
}

// confirm we have all 10 buckets
Expand Down Expand Up @@ -101,7 +101,7 @@ public void testEmptyBucketsFillIn() {
assertEquals(1, counter.buckets.size());

// wait past 3 bucket time periods (the 1st bucket then 2 empty ones)
time.increment(counter.getBucketSizeInMilliseconds() * 3);
time.increment(counter.bucketSizeInMillseconds * 3);

// add another
counter.increment(HystrixRollingNumberEvent.SUCCESS);
Expand Down Expand Up @@ -161,7 +161,7 @@ public void testTimeout() {
assertEquals(1, counter.getRollingSum(HystrixRollingNumberEvent.TIMEOUT));

// sleep to get to a new bucket
time.increment(counter.getBucketSizeInMilliseconds() * 3);
time.increment(counter.bucketSizeInMillseconds * 3);

// incremenet again in latest bucket
counter.increment(HystrixRollingNumberEvent.TIMEOUT);
Expand Down Expand Up @@ -198,7 +198,7 @@ public void testShortCircuited() {
assertEquals(1, counter.getRollingSum(HystrixRollingNumberEvent.SHORT_CIRCUITED));

// sleep to get to a new bucket
time.increment(counter.getBucketSizeInMilliseconds() * 3);
time.increment(counter.bucketSizeInMillseconds * 3);

// incremenet again in latest bucket
counter.increment(HystrixRollingNumberEvent.SHORT_CIRCUITED);
Expand Down Expand Up @@ -254,7 +254,7 @@ private void testCounterType(HystrixRollingNumberEvent type) {
assertEquals(1, counter.getRollingSum(type));

// sleep to get to a new bucket
time.increment(counter.getBucketSizeInMilliseconds() * 3);
time.increment(counter.bucketSizeInMillseconds * 3);

// increment again in latest bucket
counter.increment(type);
Expand Down Expand Up @@ -292,7 +292,7 @@ public void testIncrementInMultipleBuckets() {
counter.increment(HystrixRollingNumberEvent.SHORT_CIRCUITED);

// sleep to get to a new bucket
time.increment(counter.getBucketSizeInMilliseconds() * 3);
time.increment(counter.bucketSizeInMillseconds * 3);

// increment
counter.increment(HystrixRollingNumberEvent.SUCCESS);
Expand All @@ -319,7 +319,7 @@ public void testIncrementInMultipleBuckets() {
assertEquals(2, counter.getRollingSum(HystrixRollingNumberEvent.SHORT_CIRCUITED));

// wait until window passes
time.increment(counter.timeInMilliseconds.get());
time.increment(counter.timeInMilliseconds);

// increment
counter.increment(HystrixRollingNumberEvent.SUCCESS);
Expand Down Expand Up @@ -350,7 +350,7 @@ public void testCounterRetrievalRefreshesBuckets() {
counter.increment(HystrixRollingNumberEvent.FAILURE);

// sleep to get to a new bucket
time.increment(counter.getBucketSizeInMilliseconds() * 3);
time.increment(counter.bucketSizeInMillseconds * 3);

// we should have 1 bucket since nothing has triggered the update of buckets in the elapsed time
assertEquals(1, counter.buckets.size());
Expand All @@ -363,7 +363,7 @@ public void testCounterRetrievalRefreshesBuckets() {
assertEquals(4, counter.buckets.size());

// wait until window passes
time.increment(counter.timeInMilliseconds.get());
time.increment(counter.timeInMilliseconds);

// the total counts should all be 0 (and the buckets cleared by the get, not only increment)
assertEquals(0, counter.getRollingSum(HystrixRollingNumberEvent.SUCCESS));
Expand Down Expand Up @@ -399,7 +399,7 @@ public void testUpdateMax1() {
assertEquals(10, counter.getRollingMaxValue(HystrixRollingNumberEvent.THREAD_MAX_ACTIVE));

// sleep to get to a new bucket
time.increment(counter.getBucketSizeInMilliseconds() * 3);
time.increment(counter.bucketSizeInMillseconds * 3);

// increment again in latest bucket
counter.updateRollingMax(HystrixRollingNumberEvent.THREAD_MAX_ACTIVE, 20);
Expand Down Expand Up @@ -442,7 +442,7 @@ public void testUpdateMax2() {
assertEquals(30, counter.getRollingMaxValue(HystrixRollingNumberEvent.THREAD_MAX_ACTIVE));

// sleep to get to a new bucket
time.increment(counter.getBucketSizeInMilliseconds() * 3);
time.increment(counter.bucketSizeInMillseconds * 3);

counter.updateRollingMax(HystrixRollingNumberEvent.THREAD_MAX_ACTIVE, 30);
counter.updateRollingMax(HystrixRollingNumberEvent.THREAD_MAX_ACTIVE, 30);
Expand Down Expand Up @@ -479,17 +479,17 @@ public void testMaxValue() {
counter.updateRollingMax(type, 10);

// sleep to get to a new bucket
time.increment(counter.getBucketSizeInMilliseconds());
time.increment(counter.bucketSizeInMillseconds);

counter.updateRollingMax(type, 30);

// sleep to get to a new bucket
time.increment(counter.getBucketSizeInMilliseconds());
time.increment(counter.bucketSizeInMillseconds);

counter.updateRollingMax(type, 40);

// sleep to get to a new bucket
time.increment(counter.getBucketSizeInMilliseconds());
time.increment(counter.bucketSizeInMillseconds);

counter.updateRollingMax(type, 15);

Expand Down Expand Up @@ -535,7 +535,7 @@ public void testRolling() {
// first bucket
counter.getCurrentBucket();
try {
time.increment(counter.getBucketSizeInMilliseconds());
time.increment(counter.bucketSizeInMillseconds);
} catch (Exception e) {
// ignore
}
Expand All @@ -562,7 +562,7 @@ public void testCumulativeCounterAfterRolling() {
// first bucket
counter.increment(type);
try {
time.increment(counter.getBucketSizeInMilliseconds());
time.increment(counter.bucketSizeInMillseconds);
} catch (Exception e) {
// ignore
}
Expand Down Expand Up @@ -590,7 +590,7 @@ public void testCumulativeCounterAfterRollingAndReset() {
// first bucket
counter.increment(type);
try {
time.increment(counter.getBucketSizeInMilliseconds());
time.increment(counter.bucketSizeInMillseconds);
} catch (Exception e) {
// ignore
}
Expand Down Expand Up @@ -625,7 +625,7 @@ public void testCumulativeCounterAfterRollingAndReset2() {
// iterate over 20 buckets on a queue sized for 2
for (int i = 0; i < 20; i++) {
try {
time.increment(counter.getBucketSizeInMilliseconds());
time.increment(counter.bucketSizeInMillseconds);
} catch (Exception e) {
// ignore
}
Expand Down Expand Up @@ -660,7 +660,7 @@ public void testCumulativeCounterAfterRollingAndReset3() {
// iterate over 20 buckets on a queue sized for 2
for (int i = 0; i < 20; i++) {
try {
time.increment(counter.getBucketSizeInMilliseconds());
time.increment(counter.bucketSizeInMillseconds);
} catch (Exception e) {
// ignore
}
Expand Down