Skip to content

Commit 61152a0

Browse files
mattrjacobsmattrjacobs
committed
Merge pull request #825 from mattrjacobs/remove-bucket-resizing-rolling-number
Only allow bucket configuration to take effect on HystrixRollingNumber creation
2 parents 75f663d + dbd3226 commit 61152a0

File tree

7 files changed

+52
-50
lines changed

7 files changed

+52
-50
lines changed

‎hystrix-core/src/jmh/java/com/netflix/hystrix/perf/RollingMaxPerfTest.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,7 @@ public static class CounterState {
2424

2525
@Setup(Level.Iteration)
2626
public void setUp() {
27-
counter = new HystrixRollingNumber(
28-
HystrixProperty.Factory.asProperty(100),
29-
HystrixProperty.Factory.asProperty(10));
27+
counter = new HystrixRollingNumber(100, 10);
3028
}
3129
}
3230

‎hystrix-core/src/jmh/java/com/netflix/hystrix/perf/RollingNumberPerfTest.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,7 @@ public static class CounterState {
2424

2525
@Setup(Level.Iteration)
2626
public void setUp() {
27-
counter = new HystrixRollingNumber(
28-
HystrixProperty.Factory.asProperty(100),
29-
HystrixProperty.Factory.asProperty(10));
27+
counter = new HystrixRollingNumber(100, 10);
3028
}
3129
}
3230

‎hystrix-core/src/main/java/com/netflix/hystrix/HystrixCollapserMetrics.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ public static Collection<HystrixCollapserMetrics> getInstances() {
8888
private final HystrixRollingPercentile percentileShardSize;
8989

9090
/* package */HystrixCollapserMetrics(HystrixCollapserKey key, HystrixCollapserProperties properties) {
91-
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds(), properties.metricsRollingStatisticalWindowBuckets()));
91+
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds().get(), properties.metricsRollingStatisticalWindowBuckets().get()));
9292
this.key = key;
9393
this.properties = properties;
9494

‎hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommandMetrics.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ public static Collection<HystrixCommandMetrics> getInstances() {
137137
private final HystrixEventNotifier eventNotifier;
138138

139139
/* package */HystrixCommandMetrics(HystrixCommandKey key, HystrixCommandGroupKey commandGroup, HystrixThreadPoolKey threadPoolKey, HystrixCommandProperties properties, HystrixEventNotifier eventNotifier) {
140-
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds(), properties.metricsRollingStatisticalWindowBuckets()));
140+
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds().get(), properties.metricsRollingStatisticalWindowBuckets().get()));
141141
this.key = key;
142142
this.group = commandGroup;
143143
this.threadPoolKey = threadPoolKey;

‎hystrix-core/src/main/java/com/netflix/hystrix/HystrixThreadPoolMetrics.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ public static Collection<HystrixThreadPoolMetrics> getInstances() {
103103
private final HystrixThreadPoolProperties properties;
104104

105105
private HystrixThreadPoolMetrics(HystrixThreadPoolKey threadPoolKey, ThreadPoolExecutor threadPool, HystrixThreadPoolProperties properties) {
106-
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds(), properties.metricsRollingStatisticalWindowBuckets()));
106+
super(new HystrixRollingNumber(properties.metricsRollingStatisticalWindowInMilliseconds().get(), properties.metricsRollingStatisticalWindowBuckets().get()));
107107
this.threadPoolKey = threadPoolKey;
108108
this.threadPool = threadPool;
109109
this.properties = properties;

‎hystrix-core/src/main/java/com/netflix/hystrix/util/HystrixRollingNumber.java‎

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -52,35 +52,41 @@ public class HystrixRollingNumber {
5252

5353
private static final Time ACTUAL_TIME = new ActualTime();
5454
private final Time time;
55-
final HystrixProperty<Integer> timeInMilliseconds;
56-
final HystrixProperty<Integer> numberOfBuckets;
55+
final int timeInMilliseconds;
56+
final int numberOfBuckets;
57+
final int bucketSizeInMillseconds;
5758

5859
final BucketCircularArray buckets;
5960
private final CumulativeSum cumulativeSum = new CumulativeSum();
6061

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

65-
/* used for unit testing */
66-
/* package for testing */HystrixRollingNumber(Time time, int timeInMilliseconds, int numberOfBuckets) {
67-
this(time, HystrixProperty.Factory.asProperty(timeInMilliseconds), HystrixProperty.Factory.asProperty(numberOfBuckets));
75+
public HystrixRollingNumber(int timeInMilliseconds, int numberOfBuckets) {
76+
this(ACTUAL_TIME, timeInMilliseconds, numberOfBuckets);
6877
}
6978

70-
private HystrixRollingNumber(Time time, HystrixProperty<Integer> timeInMilliseconds, HystrixProperty<Integer> numberOfBuckets) {
79+
/* package for testing */ HystrixRollingNumber(Time time, int timeInMilliseconds, int numberOfBuckets) {
7180
this.time = time;
7281
this.timeInMilliseconds = timeInMilliseconds;
7382
this.numberOfBuckets = numberOfBuckets;
7483

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

79-
buckets = new BucketCircularArray(numberOfBuckets.get());
80-
}
81-
82-
/* package for testing */int getBucketSizeInMilliseconds() {
83-
return timeInMilliseconds.get() / numberOfBuckets.get();
89+
buckets = new BucketCircularArray(numberOfBuckets);
8490
}
8591

8692
/**
@@ -258,7 +264,7 @@ public long getRollingMaxValue(HystrixRollingNumberEvent type) {
258264
* NOTE: This is thread-safe because it's accessing 'buckets' which is a LinkedBlockingDeque
259265
*/
260266
Bucket currentBucket = buckets.peekLast();
261-
if (currentBucket != null && currentTime < currentBucket.windowStart + getBucketSizeInMilliseconds()) {
267+
if (currentBucket != null && currentTime < currentBucket.windowStart + this.bucketSizeInMillseconds) {
262268
// if we're within the bucket 'window of time' return the current one
263269
// NOTE: We do not worry if we are BEFORE the window in a weird case of where thread scheduling causes that to occur,
264270
// we'll just use the latest as long as we're not AFTER the window
@@ -299,22 +305,22 @@ public long getRollingMaxValue(HystrixRollingNumberEvent type) {
299305
} else {
300306
// We go into a loop so that it will create as many buckets as needed to catch up to the current time
301307
// as we want the buckets complete even if we don't have transactions during a period of time.
302-
for (int i = 0; i < numberOfBuckets.get(); i++) {
308+
for (int i = 0; i < numberOfBuckets; i++) {
303309
// we have at least 1 bucket so retrieve it
304310
Bucket lastBucket = buckets.peekLast();
305-
if (currentTime < lastBucket.windowStart + getBucketSizeInMilliseconds()) {
311+
if (currentTime < lastBucket.windowStart + this.bucketSizeInMillseconds) {
306312
// if we're within the bucket 'window of time' return the current one
307313
// NOTE: We do not worry if we are BEFORE the window in a weird case of where thread scheduling causes that to occur,
308314
// we'll just use the latest as long as we're not AFTER the window
309315
return lastBucket;
310-
} else if (currentTime - (lastBucket.windowStart + getBucketSizeInMilliseconds()) > timeInMilliseconds.get()) {
316+
} else if (currentTime - (lastBucket.windowStart + this.bucketSizeInMillseconds) > timeInMilliseconds) {
311317
// the time passed is greater than the entire rolling counter so we want to clear it all and start from scratch
312318
reset();
313319
// recursively call getCurrentBucket which will create a new bucket and return it
314320
return getCurrentBucket();
315321
} else { // we're past the window so we need to create a new bucket
316322
// create a new bucket and add it as the new 'last'
317-
buckets.addLast(new Bucket(lastBucket.windowStart + getBucketSizeInMilliseconds()));
323+
buckets.addLast(new Bucket(lastBucket.windowStart + this.bucketSizeInMillseconds));
318324
// add the lastBucket values to the cumulativeSum
319325
cumulativeSum.addBucket(lastBucket);
320326
}

‎hystrix-core/src/test/java/com/netflix/hystrix/util/HystrixRollingNumberTest.java‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -32,17 +32,17 @@ public void testCreatesBuckets() {
3232
try {
3333
HystrixRollingNumber counter = new HystrixRollingNumber(time, 200, 10);
3434
// confirm the initial settings
35-
assertEquals(200, counter.timeInMilliseconds.get().intValue());
36-
assertEquals(10, counter.numberOfBuckets.get().intValue());
37-
assertEquals(20, counter.getBucketSizeInMilliseconds());
35+
assertEquals(200, counter.timeInMilliseconds);
36+
assertEquals(10, counter.numberOfBuckets);
37+
assertEquals(20, counter.bucketSizeInMillseconds);
3838

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

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

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

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

106106
// add another
107107
counter.increment(HystrixRollingNumberEvent.SUCCESS);
@@ -161,7 +161,7 @@ public void testTimeout() {
161161
assertEquals(1, counter.getRollingSum(HystrixRollingNumberEvent.TIMEOUT));
162162

163163
// sleep to get to a new bucket
164-
time.increment(counter.getBucketSizeInMilliseconds() * 3);
164+
time.increment(counter.bucketSizeInMillseconds * 3);
165165

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

200200
// sleep to get to a new bucket
201-
time.increment(counter.getBucketSizeInMilliseconds() * 3);
201+
time.increment(counter.bucketSizeInMillseconds * 3);
202202

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

256256
// sleep to get to a new bucket
257-
time.increment(counter.getBucketSizeInMilliseconds() * 3);
257+
time.increment(counter.bucketSizeInMillseconds * 3);
258258

259259
// increment again in latest bucket
260260
counter.increment(type);
@@ -292,7 +292,7 @@ public void testIncrementInMultipleBuckets() {
292292
counter.increment(HystrixRollingNumberEvent.SHORT_CIRCUITED);
293293

294294
// sleep to get to a new bucket
295-
time.increment(counter.getBucketSizeInMilliseconds() * 3);
295+
time.increment(counter.bucketSizeInMillseconds * 3);
296296

297297
// increment
298298
counter.increment(HystrixRollingNumberEvent.SUCCESS);
@@ -319,7 +319,7 @@ public void testIncrementInMultipleBuckets() {
319319
assertEquals(2, counter.getRollingSum(HystrixRollingNumberEvent.SHORT_CIRCUITED));
320320

321321
// wait until window passes
322-
time.increment(counter.timeInMilliseconds.get());
322+
time.increment(counter.timeInMilliseconds);
323323

324324
// increment
325325
counter.increment(HystrixRollingNumberEvent.SUCCESS);
@@ -350,7 +350,7 @@ public void testCounterRetrievalRefreshesBuckets() {
350350
counter.increment(HystrixRollingNumberEvent.FAILURE);
351351

352352
// sleep to get to a new bucket
353-
time.increment(counter.getBucketSizeInMilliseconds() * 3);
353+
time.increment(counter.bucketSizeInMillseconds * 3);
354354

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

365365
// wait until window passes
366-
time.increment(counter.timeInMilliseconds.get());
366+
time.increment(counter.timeInMilliseconds);
367367

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

401401
// sleep to get to a new bucket
402-
time.increment(counter.getBucketSizeInMilliseconds() * 3);
402+
time.increment(counter.bucketSizeInMillseconds * 3);
403403

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

444444
// sleep to get to a new bucket
445-
time.increment(counter.getBucketSizeInMilliseconds() * 3);
445+
time.increment(counter.bucketSizeInMillseconds * 3);
446446

447447
counter.updateRollingMax(HystrixRollingNumberEvent.THREAD_MAX_ACTIVE, 30);
448448
counter.updateRollingMax(HystrixRollingNumberEvent.THREAD_MAX_ACTIVE, 30);
@@ -479,17 +479,17 @@ public void testMaxValue() {
479479
counter.updateRollingMax(type, 10);
480480

481481
// sleep to get to a new bucket
482-
time.increment(counter.getBucketSizeInMilliseconds());
482+
time.increment(counter.bucketSizeInMillseconds);
483483

484484
counter.updateRollingMax(type, 30);
485485

486486
// sleep to get to a new bucket
487-
time.increment(counter.getBucketSizeInMilliseconds());
487+
time.increment(counter.bucketSizeInMillseconds);
488488

489489
counter.updateRollingMax(type, 40);
490490

491491
// sleep to get to a new bucket
492-
time.increment(counter.getBucketSizeInMilliseconds());
492+
time.increment(counter.bucketSizeInMillseconds);
493493

494494
counter.updateRollingMax(type, 15);
495495

@@ -535,7 +535,7 @@ public void testRolling() {
535535
// first bucket
536536
counter.getCurrentBucket();
537537
try {
538-
time.increment(counter.getBucketSizeInMilliseconds());
538+
time.increment(counter.bucketSizeInMillseconds);
539539
} catch (Exception e) {
540540
// ignore
541541
}
@@ -562,7 +562,7 @@ public void testCumulativeCounterAfterRolling() {
562562
// first bucket
563563
counter.increment(type);
564564
try {
565-
time.increment(counter.getBucketSizeInMilliseconds());
565+
time.increment(counter.bucketSizeInMillseconds);
566566
} catch (Exception e) {
567567
// ignore
568568
}
@@ -590,7 +590,7 @@ public void testCumulativeCounterAfterRollingAndReset() {
590590
// first bucket
591591
counter.increment(type);
592592
try {
593-
time.increment(counter.getBucketSizeInMilliseconds());
593+
time.increment(counter.bucketSizeInMillseconds);
594594
} catch (Exception e) {
595595
// ignore
596596
}
@@ -625,7 +625,7 @@ public void testCumulativeCounterAfterRollingAndReset2() {
625625
// iterate over 20 buckets on a queue sized for 2
626626
for (int i = 0; i < 20; i++) {
627627
try {
628-
time.increment(counter.getBucketSizeInMilliseconds());
628+
time.increment(counter.bucketSizeInMillseconds);
629629
} catch (Exception e) {
630630
// ignore
631631
}
@@ -660,7 +660,7 @@ public void testCumulativeCounterAfterRollingAndReset3() {
660660
// iterate over 20 buckets on a queue sized for 2
661661
for (int i = 0; i < 20; i++) {
662662
try {
663-
time.increment(counter.getBucketSizeInMilliseconds());
663+
time.increment(counter.bucketSizeInMillseconds);
664664
} catch (Exception e) {
665665
// ignore
666666
}

0 commit comments

Comments
 (0)