diff --git a/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMerger.java b/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMerger.java index 8c7dd99dd909f..c2a88a18ad0eb 100644 --- a/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMerger.java +++ b/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMerger.java @@ -55,12 +55,21 @@ public class ExponentialHistogramMerger implements Accountable, Releasable { /** * Creates a new instance with the specified bucket limit. * - * @param bucketLimit the maximum number of buckets the result histogram is allowed to have + * @param bucketLimit the maximum number of buckets the result histogram is allowed to have, must be at least 4 * @param circuitBreaker the circuit breaker to use to limit memory allocations */ public static ExponentialHistogramMerger create(int bucketLimit, ExponentialHistogramCircuitBreaker circuitBreaker) { circuitBreaker.adjustBreaker(BASE_SIZE); - return new ExponentialHistogramMerger(bucketLimit, circuitBreaker); + boolean success = false; + try { + ExponentialHistogramMerger result = new ExponentialHistogramMerger(bucketLimit, circuitBreaker); + success = true; + return result; + } finally { + if (success == false) { + circuitBreaker.adjustBreaker(-BASE_SIZE); + } + } } private ExponentialHistogramMerger(int bucketLimit, ExponentialHistogramCircuitBreaker circuitBreaker) { @@ -69,6 +78,10 @@ private ExponentialHistogramMerger(int bucketLimit, ExponentialHistogramCircuitB // Only intended for testing, using this in production means an unnecessary reduction of precision private ExponentialHistogramMerger(int bucketLimit, int maxScale, ExponentialHistogramCircuitBreaker circuitBreaker) { + // We need at least four buckets to represent any possible distribution + if (bucketLimit < 4) { + throw new IllegalArgumentException("The bucket limit must be at least 4"); + } this.bucketLimit = bucketLimit; this.maxScale = maxScale; this.circuitBreaker = circuitBreaker; diff --git a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramGeneratorTests.java b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramGeneratorTests.java index aa9746e22d54c..24ddc0c6a7bf8 100644 --- a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramGeneratorTests.java +++ b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramGeneratorTests.java @@ -33,7 +33,7 @@ public class ExponentialHistogramGeneratorTests extends ExponentialHistogramTest public void testVeryLargeValue() { double value = Double.MAX_VALUE / 10; - ExponentialHistogram histo = createAutoReleasedHistogram(1, value); + ExponentialHistogram histo = createAutoReleasedHistogram(4, value); long index = histo.positiveBuckets().iterator().peekIndex(); int scale = histo.scale(); diff --git a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMergerTests.java b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMergerTests.java index 9b57bf5129a7a..a4b6ddd096999 100644 --- a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMergerTests.java +++ b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMergerTests.java @@ -36,6 +36,7 @@ import static org.elasticsearch.exponentialhistogram.ExponentialHistogram.MIN_INDEX; import static org.elasticsearch.exponentialhistogram.ExponentialScaleUtils.adjustScale; import static org.hamcrest.Matchers.closeTo; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; @@ -115,7 +116,7 @@ public void testAggregatesCorrectness() { try ( // Merge some empty histograms too to test that code path ReleasableExponentialHistogram merged = ExponentialHistogram.merge( - 2, + 4, breaker(), ExponentialHistogram.empty(), createAutoReleasedHistogram(10, firstValues), @@ -152,6 +153,15 @@ public void testUpscalingDoesNotExceedIndexLimits() { } } + public void testMinimumBucketCountBounded() { + try { + ExponentialHistogram.merge(3, breaker(), ExponentialHistogram.empty(), ExponentialHistogram.empty()); + fail("Expected exception"); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), containsString("limit must be at least 4")); + } + } + /** * Verify that the resulting histogram is independent of the order of elements and therefore merges performed. */ diff --git a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramUtilsTests.java b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramUtilsTests.java index 87dfb3a12ad7b..108521c4096c6 100644 --- a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramUtilsTests.java +++ b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramUtilsTests.java @@ -31,7 +31,7 @@ public class ExponentialHistogramUtilsTests extends ExponentialHistogramTestCase public void testRandomDataSumEstimation() { for (int i = 0; i < 100; i++) { int valueCount = randomIntBetween(100, 10_000); - int bucketCount = randomIntBetween(2, 500); + int bucketCount = randomIntBetween(4, 500); double correctSum = 0; double sign = randomBoolean() ? 1 : -1; diff --git a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/QuantileAccuracyTests.java b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/QuantileAccuracyTests.java index cec0d1f0f0ff7..247b0400079e2 100644 --- a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/QuantileAccuracyTests.java +++ b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/QuantileAccuracyTests.java @@ -160,7 +160,7 @@ public void testEmptyHistogram() { } public void testSingleValueHistogram() { - ExponentialHistogram histo = createAutoReleasedHistogram(1, 42.0); + ExponentialHistogram histo = createAutoReleasedHistogram(4, 42.0); for (double q : QUANTILES_TO_TEST) { assertThat(ExponentialHistogramQuantile.getQuantile(histo, q), closeTo(42, 0.0000001)); }