Skip to content

Commit

Permalink
Make values for HystrixRollingPercentile bucket calculation only affe…
Browse files Browse the repository at this point in the history 
…ct construction, and never runtime.

* This deprecates a constructor that accepted HystrixProperty<Integer> for these
  properties in favor of a new one, which accepts ints
  • Loading branch information
Matt Jacobs committed Jun 21, 2015
1 parent 793d366 commit b33622a
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 31 deletions.
6 changes: 1 addition & 5 deletions hystrix-core/src/jmh/java/com/netflix/hystrix/perf/RollingPercentilePerfTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,7 @@ public static class PercentileState {

@Setup(Level.Iteration)
public void setUp() {
percentile = new HystrixRollingPercentile(
HystrixProperty.Factory.asProperty(100),
HystrixProperty.Factory.asProperty(10),
HystrixProperty.Factory.asProperty(1000),
HystrixProperty.Factory.asProperty(percentileEnabled));
percentile = new HystrixRollingPercentile(100, 10, 1000, HystrixProperty.Factory.asProperty(percentileEnabled));
}
}

Expand Down
4 changes: 2 additions & 2 deletions hystrix-core/src/main/java/com/netflix/hystrix/HystrixCollapserMetrics.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ public static Collection<HystrixCollapserMetrics> getInstances() {
this.key = key;
this.properties = properties;

this.percentileBatchSize = new HystrixRollingPercentile(properties.metricsRollingPercentileWindowInMilliseconds(), properties.metricsRollingPercentileWindowBuckets(), properties.metricsRollingPercentileBucketSize(), properties.metricsRollingPercentileEnabled());
this.percentileShardSize = new HystrixRollingPercentile(properties.metricsRollingPercentileWindowInMilliseconds(), properties.metricsRollingPercentileWindowBuckets(), properties.metricsRollingPercentileBucketSize(), properties.metricsRollingPercentileEnabled());
this.percentileBatchSize = new HystrixRollingPercentile(properties.metricsRollingPercentileWindowInMilliseconds().get(), properties.metricsRollingPercentileWindowBuckets().get(), properties.metricsRollingPercentileBucketSize().get(), properties.metricsRollingPercentileEnabled());
this.percentileShardSize = new HystrixRollingPercentile(properties.metricsRollingPercentileWindowInMilliseconds().get(), properties.metricsRollingPercentileWindowBuckets().get(), properties.metricsRollingPercentileBucketSize().get(), properties.metricsRollingPercentileEnabled());
}

/**
Expand Down
4 changes: 2 additions & 2 deletions hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommandMetrics.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@ public static Collection<HystrixCommandMetrics> getInstances() {
this.group = commandGroup;
this.threadPoolKey = threadPoolKey;
this.properties = properties;
this.percentileExecution = new HystrixRollingPercentile(properties.metricsRollingPercentileWindowInMilliseconds(), properties.metricsRollingPercentileWindowBuckets(), properties.metricsRollingPercentileBucketSize(), properties.metricsRollingPercentileEnabled());
this.percentileTotal = new HystrixRollingPercentile(properties.metricsRollingPercentileWindowInMilliseconds(), properties.metricsRollingPercentileWindowBuckets(), properties.metricsRollingPercentileBucketSize(), properties.metricsRollingPercentileEnabled());
this.percentileExecution = new HystrixRollingPercentile(properties.metricsRollingPercentileWindowInMilliseconds().get(), properties.metricsRollingPercentileWindowBuckets().get(), properties.metricsRollingPercentileBucketSize().get(), properties.metricsRollingPercentileEnabled());
this.percentileTotal = new HystrixRollingPercentile(properties.metricsRollingPercentileWindowInMilliseconds().get(), properties.metricsRollingPercentileWindowBuckets().get(), properties.metricsRollingPercentileBucketSize().get(), properties.metricsRollingPercentileEnabled());
this.eventNotifier = eventNotifier;
}

Expand Down
63 changes: 46 additions & 17 deletions hystrix-core/src/main/java/com/netflix/hystrix/util/HystrixRollingPercentile.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,10 @@ public class HystrixRollingPercentile {
private static final Time ACTUAL_TIME = new ActualTime();
private final Time time;
/* package for testing */ final BucketCircularArray buckets;
private final HystrixProperty<Integer> timeInMilliseconds;
private final HystrixProperty<Integer> numberOfBuckets;
private final HystrixProperty<Integer> bucketDataLength;
private final int timeInMilliseconds;
private final int numberOfBuckets;
private final int bucketDataLength;
private final int bucketSizeInMilliseconds;
private final HystrixProperty<Boolean> enabled;

/*
Expand All @@ -62,39 +63,71 @@ public class HystrixRollingPercentile {
/**
*
* @param timeInMilliseconds
* {@code HystrixProperty<Integer>} for nummber of milliseconds of data that should be tracked
* {@code HystrixProperty<Integer>} for number of milliseconds of data that should be tracked
* Note that this value is represented as a {@link HystrixProperty}, but can not actually be modified
* at runtime, to avoid data loss
* <p>
* Example: 60000 for 1 minute
* @param numberOfBuckets
* {@code HystrixProperty<Integer>} for number of buckets that the time window should be divided into
* Note that this value is represented as a {@link HystrixProperty}, but can not actually be modified
* at runtime, to avoid data loss
* <p>
* Example: 12 for 5 second buckets in a 1 minute window
* @param bucketDataLength
* {@code HystrixProperty<Integer>} for number of values stored in each bucket
* Note that this value is represented as a {@link HystrixProperty}, but can not actually be modified
* at runtime, to avoid data loss
* <p>
* Example: 1000 to store a max of 1000 values in each 5 second bucket
* @param enabled
* {@code HystrixProperty<Boolean>} whether data should be tracked and percentiles calculated.
* <p>
* If 'false' methods will do nothing.
* @deprecated Please use the constructor with non-configurable properties {@link HystrixRollingPercentile(Time, int, int, int, HystrixProperty<Boolean>}
*/
@Deprecated
public HystrixRollingPercentile(HystrixProperty<Integer> timeInMilliseconds, HystrixProperty<Integer> numberOfBuckets, HystrixProperty<Integer> bucketDataLength, HystrixProperty<Boolean> enabled) {
this(timeInMilliseconds.get(), numberOfBuckets.get(), bucketDataLength.get(), enabled);
}

/**
*
* @param timeInMilliseconds
* number of milliseconds of data that should be tracked
* <p>
* Example: 60000 for 1 minute
* @param numberOfBuckets
* number of buckets that the time window should be divided into
* <p>
* Example: 12 for 5 second buckets in a 1 minute window
* @param bucketDataLength
* number of values stored in each bucket
* <p>
* Example: 1000 to store a max of 1000 values in each 5 second bucket
* @param enabled
* {@code HystrixProperty<Boolean>} whether data should be tracked and percentiles calculated.
* <p>
* If 'false' methods will do nothing.
*/
public HystrixRollingPercentile(int timeInMilliseconds, int numberOfBuckets, int bucketDataLength, HystrixProperty<Boolean> enabled) {
this(ACTUAL_TIME, timeInMilliseconds, numberOfBuckets, bucketDataLength, enabled);

}

/* package for testing */ HystrixRollingPercentile(Time time, HystrixProperty<Integer> timeInMilliseconds, HystrixProperty<Integer> numberOfBuckets, HystrixProperty<Integer> bucketDataLength, HystrixProperty<Boolean> enabled) {
/* package for testing */ HystrixRollingPercentile(Time time, int timeInMilliseconds, int numberOfBuckets, int bucketDataLength, HystrixProperty<Boolean> enabled) {
this.time = time;
this.timeInMilliseconds = timeInMilliseconds;
this.numberOfBuckets = numberOfBuckets;
this.bucketDataLength = bucketDataLength;
this.enabled = enabled;

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

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

/**
Expand Down Expand Up @@ -166,10 +199,6 @@ private PercentileSnapshot getCurrentPercentileSnapshot() {
return currentPercentileSnapshot;
}

private int getBucketSizeInMilliseconds() {
return timeInMilliseconds.get() / numberOfBuckets.get();
}

private ReentrantLock newBucketLock = new ReentrantLock();

private Bucket getCurrentBucket() {
Expand All @@ -183,7 +212,7 @@ private Bucket getCurrentBucket() {
* 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.bucketSizeInMilliseconds) {
// 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 @@ -218,29 +247,29 @@ private Bucket getCurrentBucket() {
try {
if (buckets.peekLast() == null) {
// the list is empty so create the first bucket
Bucket newBucket = new Bucket(currentTime, bucketDataLength.get());
Bucket newBucket = new Bucket(currentTime, bucketDataLength);
buckets.addLast(newBucket);
return newBucket;
} 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.bucketSizeInMilliseconds) {
// 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.bucketSizeInMilliseconds) > 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
Bucket[] allBuckets = buckets.getArray();
// create a new bucket and add it as the new 'last' (once this is done other threads will start using it on subsequent retrievals)
buckets.addLast(new Bucket(lastBucket.windowStart + getBucketSizeInMilliseconds(), bucketDataLength.get()));
buckets.addLast(new Bucket(lastBucket.windowStart + this.bucketSizeInMilliseconds, bucketDataLength));
// we created a new bucket so let's re-generate the PercentileSnapshot (not including the new bucket)
currentPercentileSnapshot = new PercentileSnapshot(allBuckets);
}
Expand Down
10 changes: 5 additions & 5 deletions hystrix-core/src/test/java/com/netflix/hystrix/util/HystrixRollingPercentileTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@

public class HystrixRollingPercentileTest {

private static final HystrixProperty<Integer> timeInMilliseconds = HystrixProperty.Factory.asProperty(60000);
private static final HystrixProperty<Integer> numberOfBuckets = HystrixProperty.Factory.asProperty(12); // 12 buckets at 5000ms each
private static final HystrixProperty<Integer> bucketDataLength = HystrixProperty.Factory.asProperty(1000);
private static final int timeInMilliseconds = 60000;
private static final int numberOfBuckets = 12; // 12 buckets at 5000ms each
private static final int bucketDataLength = 1000;
private static final HystrixProperty<Boolean> enabled = HystrixProperty.Factory.asProperty(true);

private static ExecutorService threadPool;
Expand Down Expand Up @@ -356,7 +356,7 @@ public void testDoesNothingWhenDisabled() {
@Test
public void testThreadSafety() {
final MockedTime time = new MockedTime();
final HystrixRollingPercentile p = new HystrixRollingPercentile(time, HystrixProperty.Factory.asProperty(100), HystrixProperty.Factory.asProperty(25), HystrixProperty.Factory.asProperty(1000), HystrixProperty.Factory.asProperty(true));
final HystrixRollingPercentile p = new HystrixRollingPercentile(time, 100, 25, 1000, HystrixProperty.Factory.asProperty(true));

final int NUM_THREADS = 1000;
final int NUM_ITERATIONS = 1000000;
Expand Down Expand Up @@ -408,7 +408,7 @@ public void run() {
@Test
public void testWriteThreadSafety() {
final MockedTime time = new MockedTime();
final HystrixRollingPercentile p = new HystrixRollingPercentile(time, HystrixProperty.Factory.asProperty(100), HystrixProperty.Factory.asProperty(25), HystrixProperty.Factory.asProperty(1000), HystrixProperty.Factory.asProperty(true));
final HystrixRollingPercentile p = new HystrixRollingPercentile(time, 100, 25, 1000, HystrixProperty.Factory.asProperty(true));

final int NUM_THREADS = 10;
final int NUM_ITERATIONS = 1000;
Expand Down

0 comments on commit b33622a

Please sign in to comment.