CNDB-13617: EstimatedHistogram uses C* bucket boundaries regardless of the DSE-compatibility flag (#1725)

jakubzytka · web-flow · commit f7a70ff16221 · 2025-05-28T12:41:08.000+02:00
### What is the issue
When using `use_dse_compatible_histogram_boundaries` flag we by mistake
use DSE DecayingEstimatedHistogramReservoir bucket boundaries for
`EstimatedHistogram`. This may lead to unexpected and unhandled
histogram overflow for extremely large partitions.

### What does this PR fix and why was it fixed
This fix makes `EstimatedHistogram` use, by default, the same bucket
boundaries as in upstream Cassandra and DSE `EstimatedHistogram`
diff --git a/src/java/org/apache/cassandra/io/sstable/metadata/MetadataCollector.java b/src/java/org/apache/cassandra/io/sstable/metadata/MetadataCollector.java
@@ -56,7 +56,8 @@ static EstimatedHistogram defaultCellPerPartitionCountHistogram()
 
     static EstimatedHistogram defaultPartitionSizeHistogram()
     {
-        // EH of 150 can track a max value of 1697806495183, i.e., > 1.5PB
+        // EH of 150 can track a max value of 1414838745986, i.e., ~ 1.5PB
+        // see {@link MetadataCollectorTest#testFindMaxSampleWithoutOverflow} for details
         return new EstimatedHistogram(150);
     }
 
diff --git a/src/java/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoir.java b/src/java/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoir.java
@@ -38,7 +38,6 @@
 
 import static java.lang.Math.max;
 import static java.lang.Math.min;
-import static org.apache.cassandra.utils.EstimatedHistogram.USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES;
 
 /**
  * A decaying histogram reservoir where values collected during each minute will be twice as significant as the values
@@ -81,6 +80,7 @@
  */
 public class DecayingEstimatedHistogramReservoir implements Reservoir
 {
+    public static final boolean USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES = CassandraRelevantProperties.USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES.getBoolean();
     /**
      * The default number of decayingBuckets. Use this bucket count to reduce memory allocation for bucket offsets.
      */
@@ -224,7 +224,10 @@ public DecayingEstimatedHistogramReservoir(Clock clock)
         }
         else
         {
-            bucketOffsets = EstimatedHistogram.newOffsets(bucketCount, considerZeroes);
+            if (USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES)
+                bucketOffsets = newDseOffsets(bucketCount, considerZeroes);
+            else
+                bucketOffsets = EstimatedHistogram.newOffsets(bucketCount, considerZeroes);
         }
 
         nStripes = stripes;
diff --git a/src/java/org/apache/cassandra/utils/EstimatedHistogram.java b/src/java/org/apache/cassandra/utils/EstimatedHistogram.java
@@ -36,7 +36,6 @@
 public class EstimatedHistogram
 {
     public static final EstimatedHistogramSerializer serializer = new EstimatedHistogramSerializer();
-    public static final boolean USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES = CassandraRelevantProperties.USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES.getBoolean();
 
     /**
      * The series of values to which the counts in `buckets` correspond:
@@ -91,10 +90,7 @@ public EstimatedHistogram(long[] offsets, long[] bucketData)
 
     public static long[] newOffsets(int size, boolean considerZeroes)
     {
-        if (USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES)
-            return DecayingEstimatedHistogramReservoir.newDseOffsets(size, considerZeroes);
-        else
-            return newCassandraOffsets(size, considerZeroes);
+        return newCassandraOffsets(size, considerZeroes);
     }
 
     public static long[] newCassandraOffsets(int size, boolean considerZeroes)
diff --git a/test/unit/org/apache/cassandra/io/sstable/metadata/MetadataCollectorTest.java b/test/unit/org/apache/cassandra/io/sstable/metadata/MetadataCollectorTest.java
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.io.sstable.metadata;
+
+import org.apache.cassandra.config.CassandraRelevantProperties;
+import org.apache.cassandra.utils.EstimatedHistogram;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.cassandra.io.sstable.metadata.MetadataCollector.defaultPartitionSizeHistogram;
+import static org.junit.Assert.*;
+
+public class MetadataCollectorTest
+{
+    private static final Logger logger = LoggerFactory.getLogger(MetadataCollectorTest.class);
+
+    @Test
+    public void testNoOverflow()
+    {
+        EstimatedHistogram histogram = defaultPartitionSizeHistogram();
+        histogram.add(1414838745986L);
+        assertFalse(histogram.isOverflowed());
+    }
+
+    @Test
+    public void testFindMaxSampleWithoutOverflow()
+    {
+        logger.info("dse compatible boundaries: {}", CassandraRelevantProperties.USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES.getBoolean());
+
+        long low = 0;
+        long high = Long.MAX_VALUE;
+        long result = -1;
+
+        while (low <= high) {
+            long mid = low + (high - low) / 2;  // Avoid potential overflow in (low + high) / 2
+
+            // Create a fresh histogram for each test to avoid accumulated state
+            EstimatedHistogram testHistogram = defaultPartitionSizeHistogram();
+            testHistogram.add(mid);
+
+            if (testHistogram.isOverflowed()) {
+                high = mid - 1;
+            } else {
+                result = mid;  // Keep track of the last working value
+                low = mid + 1;
+            }
+        }
+
+        logger.info("Max value without overflow: {}", result);
+
+        // Verify the result
+        EstimatedHistogram finalHistogram = defaultPartitionSizeHistogram();
+        finalHistogram.add(result);
+        assertFalse(finalHistogram.isOverflowed());
+
+        // Verify that result + 1 causes overflow
+        EstimatedHistogram overflowHistogram = defaultPartitionSizeHistogram();
+        overflowHistogram.add(result + 1);
+        assertTrue(overflowHistogram.isOverflowed());
+    }
+}
diff --git a/test/unit/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoirTestBase.java b/test/unit/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoirTestBase.java
@@ -44,6 +44,7 @@
 
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.apache.cassandra.metrics.DecayingEstimatedHistogramReservoir.DEFAULT_STRIPE_COUNT;
 import static org.apache.cassandra.metrics.DecayingEstimatedHistogramReservoir.MAX_BUCKET_COUNT;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -77,7 +78,13 @@ private static Gen<long[]> generateOffsets()
     {
         return integers().from(DecayingEstimatedHistogramReservoir.DEFAULT_BUCKET_COUNT)
                          .upToAndIncluding(DecayingEstimatedHistogramReservoir.MAX_BUCKET_COUNT - 10)
-                         .zip(booleans().all(), EstimatedHistogram::newOffsets);
+                         .zip(booleans().all(), DecayingEstimatedHistogramReservoirTestBase::decayingHistogramOffsets);
+    }
+
+    private static long[] decayingHistogramOffsets(int bucketsCount, boolean considerZeroes)
+    {
+        return ((DecayingEstimatedHistogramReservoir.EstimatedHistogramReservoirSnapshot)
+                new DecayingEstimatedHistogramReservoir(considerZeroes, bucketsCount, DEFAULT_STRIPE_COUNT).getSnapshot()).getOffsets();
     }
 
     @Test
@@ -86,6 +93,7 @@ public void testFindIndex()
         qt().withExamples(numExamples)
             .forAll(booleans().all()
                               .flatMap(b -> offsets.flatMap(offs -> this.offsetsAndValue(offs, b, 0))))
+                              .describedAs(pair -> String.format("offsets=%s, value=%d", Arrays.toString(pair.left), pair.right))
             .check(this::checkFindIndex);
     }
 
@@ -309,7 +317,7 @@ public void testMean()
 
             DecayingEstimatedHistogramReservoir histogram = new DecayingEstimatedHistogramReservoir(true,
                                                                                                     DecayingEstimatedHistogramReservoir.DEFAULT_BUCKET_COUNT,
-                                                                                                    DecayingEstimatedHistogramReservoir.DEFAULT_STRIPE_COUNT,
+                                                                                                    DEFAULT_STRIPE_COUNT,
                                                                                                     clock);
             for (int i = 0; i < 40; i++)
                 histogram.update(0);
@@ -425,7 +433,7 @@ public void testPercentile()
 
             DecayingEstimatedHistogramReservoir histogram = new DecayingEstimatedHistogramReservoir(true,
                                                                                                     DecayingEstimatedHistogramReservoir.DEFAULT_BUCKET_COUNT,
-                                                                                                    DecayingEstimatedHistogramReservoir.DEFAULT_STRIPE_COUNT,
+                                                                                                    DEFAULT_STRIPE_COUNT,
                                                                                                     clock);
             histogram.update(0);
             histogram.update(0);
diff --git a/test/unit/org/apache/cassandra/utils/DSEEstimatedHistogramTest.java b/test/unit/org/apache/cassandra/utils/DSEEstimatedHistogramTest.java
@@ -39,25 +39,36 @@ public static void setup()
     @Test
     public void testDSEBoundaries()
     {
-            // these boundaries were computed in DSE
-            long[] dseBoundaries = new long[]{ 1, 2, 3, 4, 5, 6, 7, 8, 10, 12, 14, 16, 20, 24, 28, 32, 40,
-                                               48, 56, 64, 80, 96, 112, 128, 160, 192, 224, 256, 320, 384, 448, 512, 640, 768, 896, 1024, 1280, 1536, 1792,
-                                               2048, 2560, 3072, 3584, 4096, 5120, 6144, 7168, 8192, 10240, 12288, 14336, 16384, 20480, 24576, 28672, 32768,
-                                               40960, 49152, 57344, 65536, 81920, 98304, 114688, 131072, 163840, 196608, 229376, 262144, 327680, 393216,
-                                               458752, 524288, 655360, 786432, 917504, 1048576, 1310720, 1572864, 1835008, 2097152, 2621440, 3145728, 3670016,
-                                               4194304, 5242880, 6291456, 7340032, 8388608, 10485760, 12582912, 14680064, 16777216, 20971520, 25165824,
-                                               29360128, 33554432, 41943040, 50331648, 58720256, 67108864, 83886080, 100663296, 117440512, 134217728,
-                                               167772160, 201326592, 234881024, 268435456, 335544320, 402653184, 469762048, 536870912, 671088640, 805306368,
-                                               939524096, 1073741824, 1342177280, 1610612736, 1879048192, 2147483648L, 2684354560L, 3221225472L, 3758096384L,
-                                               4294967296L, 5368709120L, 6442450944L, 7516192768L, 8589934592L, 10737418240L, 12884901888L, 15032385536L, 17179869184L,
-                                               21474836480L, 25769803776L, 30064771072L, 34359738368L, 42949672960L, 51539607552L, 60129542144L, 68719476736L,
-                                               85899345920L, 103079215104L, 120259084288L, 137438953472L, 171798691840L, 206158430208L, 240518168576L, 274877906944L,
-                                               343597383680L, 412316860416L, 481036337152L, 549755813888L, 687194767360L, 824633720832L, 962072674304L, 1099511627776L,
-                                               1374389534720L, 1649267441664L, 1924145348608L, 2199023255552L, 2748779069440L, 3298534883328L, 3848290697216L,
-                                               4398046511104L, 5497558138880L, 6597069766656L, 7696581394432L, 8796093022208L, 10995116277760L, 13194139533312L,
-                                               15393162788864L, 17592186044416L, 21990232555520L, 26388279066624L, 30786325577728L, 35184372088832L, 43980465111040L,
-                                               52776558133248L, 61572651155456L, 70368744177664L, 87960930222080L, 105553116266496L, 123145302310912L,
-                                               140737488355328L, 175921860444160L };
+            // these boundaries were computed in DSE, in HistogramSnapshotTest::boundaries
+            long[] dseBoundaries = new long[]{ 1, 2, 3, 4, 5, 6, 7, 8, 10, 12, 14, 17, 20, 24, 29, 35, 42, 50, 60, 72, 86, 103, 124,
+                149, 179, 215, 258, 310, 372, 446, 535, 642, 770, 924, 1109, 1331, 1597, 1916, 2299, 2759, 3311, 3973, 4768,
+                5722, 6866, 8239, 9887, 11864, 14237, 17084, 20501, 24601, 29521, 35425, 42510, 51012, 61214, 73457, 88148,
+                105778, 126934, 152321, 182785, 219342, 263210, 315852, 379022, 454826, 545791, 654949, 785939, 943127,
+                1131752, 1358102, 1629722, 1955666, 2346799, 2816159, 3379391, 4055269, 4866323, 5839588, 7007506, 8409007,
+                10090808, 12108970, 14530764, 17436917, 20924300, 25109160, 30130992, 36157190, 43388628, 52066354, 62479625,
+                74975550, 89970660, 107964792, 129557750, 155469300, 186563160, 223875792, 268650950, 322381140, 386857368,
+                464228842, 557074610, 668489532, 802187438, 962624926, 1155149911, 1386179893, 1663415872, 1996099046,
+                2395318855L, 2874382626L, 3449259151L, 4139110981L, 4966933177L, 5960319812L, 7152383774L, 8582860529L,
+                10299432635L, 12359319162L, 14831182994L, 17797419593L, 21356903512L, 25628284214L, 30753941057L,
+                36904729268L, 44285675122L, 53142810146L, 63771372175L, 76525646610L, 91830775932L, 110196931118L,
+                132236317342L, 158683580810L, 190420296972L, 228504356366L, 274205227639L, 329046273167L, 394855527800L,
+                473826633360L, 568591960032L, 682310352038L, 818772422446L, 982526906935L, 1179032288322L, 1414838745986L,
+                1697806495183L, 2037367794220L, 2444841353064L, 2933809623677L, 3520571548412L, 4224685858094L,
+                5069623029713L, 6083547635656L, 7300257162787L, 8760308595344L, 10512370314413L, 12614844377296L,
+                15137813252755L, 18165375903306L, 21798451083967L, 26158141300760L, 31389769560912L, 37667723473094L,
+                45201268167713L, 54241521801256L, 65089826161507L, 78107791393808L, 93729349672570L, 112475219607084L,
+                134970263528501L, 161964316234201L, 194357179481041L, 233228615377249L, 279874338452699L, 335849206143239L,
+                403019047371887L, 483622856846264L, 580347428215517L, 696416913858620L, 835700296630344L, 1002840355956413L,
+                1203408427147696L, 1444090112577235L, 1732908135092682L, 2079489762111218L, 2495387714533462L,
+                2994465257440155L, 3593358308928186L, 4312029970713823L, 5174435964856587L, 6209323157827904L,
+                7451187789393485L, 8941425347272182L, 10729710416726618L, 12875652500071942L, 15450783000086330L,
+                18540939600103596L, 22249127520124316L, 26698953024149180L, 32038743628979016L, 38446492354774816L,
+                46135790825729776L, 55362948990875728L, 66435538789050872L, 79722646546861040L, 95667175856233248L,
+                114800611027479888L, 137760733232975856L, 165312879879571008L, 198375455855485216L, 238050547026582240L,
+                285660656431898688L, 342792787718278400L, 411351345261934080L, 493621614314320896L, 592345937177185024L,
+                710815124612621952L, 852978149535146368L, 1023573779442175616L, 1228288535330610688L, 1473946242396732672L,
+                1768735490876079104L, 2122482589051294720L, 2546979106861553664L, 3056374928233864192L, 3667649913880636928L,
+                4401179896656763904L, 5281415875988116480L, 6337699051185739776L, 7605238861422887936L, 9126286633707464704L };
 
             // the code below is O(n^2) so that we don't need to assume that boundaries are independent
             // of the histogram size; this is not a problem since the number of boundaries is small

Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,8 @@ static EstimatedHistogram defaultCellPerPartitionCountHistogram()`
`56`	`56`
`57`	`57`	`static EstimatedHistogram defaultPartitionSizeHistogram()`
`58`	`58`	`{`
`59`		`- // EH of 150 can track a max value of 1697806495183, i.e., > 1.5PB`
	`59`	`+ // EH of 150 can track a max value of 1414838745986, i.e., ~ 1.5PB`
	`60`	`+ // see {@link MetadataCollectorTest#testFindMaxSampleWithoutOverflow} for details`
`60`	`61`	`return new EstimatedHistogram(150);`
`61`	`62`	`}`
`62`	`63`
Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,6 @@`
`38`	`38`
`39`	`39`	`import static java.lang.Math.max;`
`40`	`40`	`import static java.lang.Math.min;`
`41`		`-import static org.apache.cassandra.utils.EstimatedHistogram.USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES;`
`42`	`41`
`43`	`42`	`/**`
`44`	`43`	`* A decaying histogram reservoir where values collected during each minute will be twice as significant as the values`
`@@ -81,6 +80,7 @@`
`81`	`80`	`*/`
`82`	`81`	`public class DecayingEstimatedHistogramReservoir implements Reservoir`
`83`	`82`	`{`
	`83`	`+ public static final boolean USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES = CassandraRelevantProperties.USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES.getBoolean();`
`84`	`84`	`/**`
`85`	`85`	`* The default number of decayingBuckets. Use this bucket count to reduce memory allocation for bucket offsets.`
`86`	`86`	`*/`
`@@ -224,7 +224,10 @@ public DecayingEstimatedHistogramReservoir(Clock clock)`
`224`	`224`	`}`
`225`	`225`	`else`
`226`	`226`	`{`
`227`		`- bucketOffsets = EstimatedHistogram.newOffsets(bucketCount, considerZeroes);`
	`227`	`+ if (USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES)`
	`228`	`+ bucketOffsets = newDseOffsets(bucketCount, considerZeroes);`
	`229`	`+ else`
	`230`	`+ bucketOffsets = EstimatedHistogram.newOffsets(bucketCount, considerZeroes);`
`228`	`231`	`}`
`229`	`232`
`230`	`233`	`nStripes = stripes;`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,6 @@`
`36`	`36`	`public class EstimatedHistogram`
`37`	`37`	`{`
`38`	`38`	`public static final EstimatedHistogramSerializer serializer = new EstimatedHistogramSerializer();`
`39`		`- public static final boolean USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES = CassandraRelevantProperties.USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES.getBoolean();`
`40`	`39`
`41`	`40`	`/**`
`42`	`41`	* The series of values to which the counts in `buckets` correspond:
`@@ -91,10 +90,7 @@ public EstimatedHistogram(long[] offsets, long[] bucketData)`
`91`	`90`
`92`	`91`	`public static long[] newOffsets(int size, boolean considerZeroes)`
`93`	`92`	`{`
`94`		`- if (USE_DSE_COMPATIBLE_HISTOGRAM_BOUNDARIES)`
`95`		`- return DecayingEstimatedHistogramReservoir.newDseOffsets(size, considerZeroes);`
`96`		`- else`
`97`		`- return newCassandraOffsets(size, considerZeroes);`
	`93`	`+ return newCassandraOffsets(size, considerZeroes);`
`98`	`94`	`}`
`99`	`95`
`100`	`96`	`public static long[] newCassandraOffsets(int size, boolean considerZeroes)`