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

Jump to bottom

Make values for HystrixRollingPercentile bucket calculation only affect construction #824

Merged
merged 1 commit into from
Jun 21, 2015
Merged

Make values for HystrixRollingPercentile bucket calculation only affect construction #824

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
6 changes: 1 addition & 5 deletions hystrix-core/src/jmh/java/com/netflix/hystrix/perf/RollingPercentilePerfTest.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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