CASSANDRA-18397: Unified Compaction Strategy #2287

blambov · 2023-04-20T14:15:15Z

Implementation of the Unified Compaction Strategy as described in CEP-26.

The approach is documented in the included UnifiedCompactionStrategy.md.

The patch is best reviewed by individual commits, which split the preparation and enhancements into smaller chunks. The main contribution is in 0715a66.

Maxwell-Guo

complete the first round

Maxwell-Guo · 2023-04-21T03:10:14Z

src/java/org/apache/cassandra/db/compaction/UnifiedCompactionStrategy.java

+    private static final Pattern SCALING_PARAMETER_PATTERN = Pattern.compile("(N)|L(\\d+)|T(\\d+)|([+-]?\\d+)");
+    private static final String SCALING_PARAMETER_PATTERN_SIMPLIFIED = SCALING_PARAMETER_PATTERN.pattern()
+                                                                                                .replaceAll("[()]", "")
+


it seems there is an empty line.

Maxwell-Guo · 2023-06-26T02:39:13Z

src/java/org/apache/cassandra/config/CassandraRelevantProperties.java

@@ -516,6 +517,11 @@ public enum CassandraRelevantProperties
    TRIGGERS_DIR("cassandra.triggers_dir"),
    TRUNCATE_BALLOT_METADATA("cassandra.truncate_ballot_metadata"),
    TYPE_UDT_CONFLICT_BEHAVIOR("cassandra.type.udt.conflict_behavior"),
+    UCS_BASE_SHARD_COUNT("unified_compaction.base_shard_count", "4"),


do we need to add some descriptions for these UCS configuration?

Added a reference to Controller, where they are defined and described.

Maxwell-Guo · 2023-06-26T02:41:22Z

src/java/org/apache/cassandra/config/CassandraRelevantProperties.java

+     * Gets the value of a system property as a double.
+     * @return system property long value if it exists, defaultValue otherwise.
+     */
+    public double getLong(double overrideDefaultValue)


do we need to change the function name ? the description is get the value as a double but the funtion name is getLong ? this is confusing. May be change to getDouble(double overrideDefaultValue)

Yes, changed.

Maxwell-Guo · 2023-06-26T03:36:18Z

src/java/org/apache/cassandra/config/Config.java

+    /**
+     * Default compaction configuration, used if a table does not specify any.
+     */
+    public ParameterizedClass default_compaction = null;


I think the parameter name should be changed to default_compaction_for_table,or something else.

we should add some detailed description in yaml(But I do not saw any information change in cassandra.yaml in this patch), because we may modify the user's default behavior, which needs to be perceived by the user. In the old way if a use create a table without specify the compaction strategy , the STCS is used, but now the strategy may changed if this paramter has been set to other compaction strategy.

Added an entry in cassandra.yaml.

The flag specifies a default value for the compaction parameter. Could you elaborate on why it should be renamed?

The flag specifies a default value for the compaction parameter. Could you elaborate on why it should be renamed?

Actually, it feels like there's no need to change it

Maxwell-Guo · 2023-06-26T03:54:54Z

src/java/org/apache/cassandra/db/compaction/ShardManager.java

+     */
+    default double rangeSpanned(SSTableReader rdr)
+    {
+        double reported = rdr.tokenSpaceCoverage();


what about change to
double span = (reported > 0) ? reported : rangeSpanned(rdr.getFirst(), rdr.getLast());

The full form is slightly easier to read.

Maxwell-Guo · 2023-06-26T04:55:42Z

test/unit/org/apache/cassandra/db/compaction/unified/ControllerTest.java

@@ -0,0 +1,336 @@
+/*
+ * Copyright DataStax, Inc.


remove this

Maxwell-Guo · 2023-06-26T04:55:54Z

test/unit/org/apache/cassandra/db/compaction/UnifiedCompactionStrategyTest.java

@@ -0,0 +1,913 @@
+/*
+ * Copyright DataStax, Inc.


remove this

Maxwell-Guo · 2023-06-26T04:56:53Z

test/unit/org/apache/cassandra/db/compaction/UnifiedCompactionStrategyTest.java

+        List<SSTableReader> allSstables = new ArrayList<>();
+
+        List<SSTableReader> sstables = mockSSTables(numSSTables,
+//                                                    minSstableSizeBytes,


I think it is better to remove this line instand of comment out this line

Maxwell-Guo · 2023-06-26T04:57:25Z

test/unit/org/apache/cassandra/db/compaction/unified/ShardedCompactionWriterTest.java

@@ -0,0 +1,295 @@
+/*
+ * Copyright DataStax, Inc.


remove this line

Maxwell-Guo · 2023-06-26T04:58:04Z

test/unit/org/apache/cassandra/db/compaction/unified/ShardedMultiWriterTest.java

@@ -0,0 +1,144 @@
+/*
+ * Copyright DataStax, Inc.


remove this line

Gerrrr

First pass. I hope to do another pass later this week with more meaningful comments.

Gerrrr · 2023-06-19T23:25:48Z

src/java/org/apache/cassandra/dht/Splitter.java

            while (sum.add(currentRangeWidth).compareTo(perPart) >= 0)
            {
                BigInteger withinRangeBoundary = perPart.subtract(sum);
-                left = left.add(withinRangeBoundary);
+                left = left.add(withinRangeBoundary.multiply(currentRangeFactor));


Does this deserve a test that demonstrates the issue?

This came up as a result of a failure of ShardManagerTest.testShardBoundariesWeighted.

Added a more direct test in SplitterTest.testWithWeight.

Gerrrr · 2023-06-26T18:50:55Z

src/java/org/apache/cassandra/db/compaction/UnifiedCompactionStrategy.java

+                                                       Collection<Index> indexes,
+                                                       LifecycleNewTracker lifecycleNewTracker)
+    {
+        // FIXME: needs the metadata collector fix


Does this comment refer to 2558dfb?

Yes, comment removed.

Gerrrr · 2023-06-26T18:59:39Z

src/java/org/apache/cassandra/db/compaction/UnifiedCompactionStrategy.java

+        Level level = new Level(controller, index, 0, maxSize);
+        for (SSTableReader candidate : suitable)
+        {
+            final double size = shardManager.density(candidate);


nit: we should probably rename local variables such as maxSize and size to maxDensity and density to avoid confusion.

Gerrrr · 2023-06-26T18:59:52Z

src/java/org/apache/cassandra/db/compaction/UnifiedCompactionStrategy.java

+
+    /**
+     * Groups the sstables passed in into arenas and buckets. This is used by the strategy to determine
+     * new compactions, and by external tools in CNDB to analyze the strategy decisions.


nit: javadoc references CNDB

Does CNDB is some thing that developed by datastax ? I have saw this keyword in one of the patchs of sai

CNDB is in internal name for DataStax's Astra DB. Removed reference.

Gerrrr · 2023-06-26T19:04:40Z

src/java/org/apache/cassandra/db/compaction/UnifiedCompactionStrategy.java

+        final int threshold; // number of SSTables that trigger a compaction
+        final double min; // min density of sstables for this level
+        final double max; // max density of sstables for this level
+        int maxOverlap = -1; // maximum number of overlapping sstables


nit: Can you please add a line on why one would want to limit the overlap? What happens if a level/bucket crosses this threshold? Do we not run a compaction in that level? How do we recover?

Added a line here and an explanation of the process in getCompactionPick as well as getBuckets.

Gerrrr · 2023-06-26T19:18:17Z

src/java/org/apache/cassandra/db/compaction/unified/ShardedMultiWriter.java

+ * shards used by {@link org.apache.cassandra.db.compaction.UnifiedCompactionStrategy} as long as the size of
+ * the sstable so far is sufficiently large.
+ * <p/>
+ * This is class is similar to {@link ShardedMultiWriter} but for flushing. Unfortunately


Similar to some other multi-writer?

Gerrrr · 2023-06-26T23:19:31Z

Q: does this patch require any changes or additional test cases in dtests?

Maxwell-Guo · 2023-06-27T02:13:56Z

I think there is no need for a dtest,because compaction is a single node behavior.

But Is it necessary to add a tests that we alter schema from stcs/lcs/twcs to UCS ? Will the alter behavior result meet expectations?

blambov · 2023-06-28T12:33:44Z

There is an in-JVM dtest (UnifiedCompactionDensitiesTest) for the part that can be affected by local ownership.
Another reason I would use a dtest is to check behaviour on shutting down and restarting, but in this case the strategy is stateless and should not need it.

blambov · 2023-07-18T14:16:33Z

Pushed a fix for one flaky test and added a dtest pull request to correct a test for the more precise printout in bytes. The remaining repeated test failures of the corrupted compaction test are due to not treating some kinds of data corruption as such. The fix for this is somewhat invasive and needs to be done in a separate ticket (the problem is not introduced by this patch).

@Maxwell-Guo, do you need any further changes? If not, could you approve the PR?

Maxwell-Guo · 2023-07-18T15:07:41Z

Hi @blambov I want to do the last round of review tomorrow before +1, is that ok? I've been a little busy these days, sorry

Introduces prettyPrintBinary (with 2^10 multiplier, e.g. MiB) and prettyPrintDecimal (with 1000 multiplier, e.g. ms) that cover the full range of double, and a corresponding parsing method that can read both.

If a compaction strategy splits sstables during flush, this can cause the cardinality information of an sstable to include more keys than it actually has, resulting in grossly overestimated key counts when picking a bloom filter size; the latter can result in several times larger bloom filters on L1 than they should be.

This is needed to avoid situations where changed local token space ownership causes SSTables to have their density calculated incorrectly. This is only calculated and written by UCS writers; when the value is not available in the file, it is calculated from the covered range.

and switches to UCS for the trie unit test suite

Store targetSSTableSize and apply sqrt(0.5) in calculation.

Fix test failures

Maxwell-Guo

only some small nits ,+1 if final CI results are green

Maxwell-Guo · 2023-07-19T05:39:33Z

conf/cassandra.yaml

+# Default compaction strategy, applied when a table's parameters do not
+# specify compaction.
+# The default is to use SizeTieredCompactionStrategy, with its default
+# threshold of 4 sstables.


The default strategy of STCS here should not only include the lower limit of 4, but also the upper limit of 32, so
what about just modify the description to :
“The default is to use SizeTieredCompactionStrategy with its defaut compaction params.”
It feels more ordinary

Maxwell-Guo · 2023-07-19T05:50:44Z

conf/cassandra.yaml

+# threshold of 4 sstables.
+# The selected compaction strategy will also apply to system tables.
+#
+# default_compaction:


As this is a newly introduced configuration, so I think we should give a more detailed description abouth the usage, like :
class_name should be the different compaction strategy name , parameters sholud be the maps of differen parameters that different compaction strategy used. Then the next is an example

I started changing this to:

# Default compaction strategy, applied when a table's parameters do not # specify compaction. A compaction "class_name" must be given, and optionally # its class-specific configuration parameters under "parameters" as in the # example below. # The selected compaction strategy will also apply to system tables. # # The default is to use SizeTieredCompactionStrategy, with its default # compaction parameters.

but then realized this is out of line with other such items in the YAML, which give the configuration by example only. E.g. commitlog_compression, hints_compression are described similarly and go through the same transformation (class among other parameters in the schema to class_name and parameters fields in the YAML).

I'm leaning towards keeping it short and in line with other items, but I'm happy to change it if you prefer.

ok，then I think keep same with "commitlog_compression" or "hints_compression" is enough.

Maxwell-Guo · 2023-07-19T06:22:11Z

src/java/org/apache/cassandra/db/compaction/UnifiedCompactionStrategy.md

@@ -0,0 +1,375 @@
+<!--
+#
+# Copyright DataStax, Inc.


I have missed this before ~~~ line 1 to line 3
I think we should modify these lines.

Shoot, I thought I changed all these. Fixed now.

I did another search for "DataStax" and nothing license-related appeared.

Maxwell-Guo · 2023-07-19T06:30:11Z

src/java/org/apache/cassandra/db/compaction/unified/ShardedMultiWriter.java

+        for (SSTableWriter writer : writers)
+            if (writer != null)
+                return writer.getFilename();
+        return "";


should file name be "" if all writer is null ?

I think so. This is used for logging, and reporting no filename with 0B written is sensible.

Maxwell-Guo · 2023-07-19T08:41:52Z

conf/cassandra.yaml

+# threshold of 4 sstables.
+# The selected compaction strategy will also apply to system tables.
+#
+# default_compaction:


ok，then I think keep same with "commitlog_compression" or "hints_compression" is enough.

blambov · 2023-07-20T12:09:30Z

CI j11: Repeated tests failing due to CASSANDRA-18676 plus another known flake.
CI j8: Repeated tests failing due to CASSANDRA-18676, plus testPagingWithClustering-.jdk11 and a flake of randomSplitTestVNodesMurmur3Partitioner-compression.jdk11.

Taking another look if the latter two can be caused by this patch.

blambov · 2023-07-20T12:49:29Z

randomSplitTestVNodesMurmur3Partitioner-compression.jdk11 uses only a weight of 1.0 and thus executes exactly the same code as before (as BigInteger.multiply(new BigInteger(1)) boils down to copying).
testPagingWithClustering-.jdk11 didn't fail in previous runs and in this case it's not running with UCS. I can't think of a way that it could be related.

I'm going to commit the patch and continue with CASSANDRA-18676.

blambov · 2023-07-20T16:41:02Z

Committed as 957eca2.

blambov force-pushed the CASSANDRA-18397 branch from f8775d7 to 541fe1b Compare April 20, 2023 14:17

blambov force-pushed the CASSANDRA-18397 branch from cb75378 to 78064b6 Compare June 20, 2023 16:00

Maxwell-Guo reviewed Jun 26, 2023

View reviewed changes

Gerrrr reviewed Jun 26, 2023

View reviewed changes

Gerrrr approved these changes Jul 11, 2023

View reviewed changes

blambov force-pushed the CASSANDRA-18397 branch from fb343de to 899c6eb Compare July 17, 2023 13:12

blambov added 19 commits July 18, 2023 23:54

Generalize prettyPrint, add parseHumanReadable and test

dabda81

Introduces prettyPrintBinary (with 2^10 multiplier, e.g. MiB) and prettyPrintDecimal (with 1000 multiplier, e.g. ms) that cover the full range of double, and a corresponding parsing method that can read both.

Rename increaseSlightly to nextValidToken

5b91a01

Rename sstableComparator to firstKeyComparator

712527a

Encapsulate SSTableReader first and last

1318f1d

Introduce CFS.getKeyspaceName()

e814929

Use SSTableReader.getMaxLocalDeletionTime

3be1fd4

Remove unused version of SSTableMultiWriter.finish

f9a2d0e

Fix code repetition in compaction writers

a0d68b6

Fix weighted splitting

d4cf1f3

Add flushSizeOnDisk metric

e5582c3

UnifiedCompactionStrategy, CEP-26

fec6e8b

Work around CASSANDRA-18342

c095067

Adds ability to change compaction default in YAML

9197ba7

and switches to UCS for the trie unit test suite

Trivial corrections

e2295f4

Test and fix shard number calculation for multiple nodes/disks

13a71ae

Set max_sstables_to_compact default to 32

b2835f2

Store targetSSTableSize and apply sqrt(0.5) in calculation.

Fix header

41defe6

blambov added 7 commits July 18, 2023 23:54

Review comments

6447708

Review comments

fa86c79

Switch to K instead of k for kilo

08384a9

Fix test failures

Tweak flaky test for more information

a38d50f

Fix flaky DiskSpaceMetricsTest

6b7e755

review nits

58ba8f1

DO NOT COMMIT: CircleCI config

f586649

Maxwell-Guo approved these changes Jul 19, 2023

View reviewed changes

Added CHANGES.txt and NEWS.txt entries

920022c

Maxwell-Guo approved these changes Jul 19, 2023

View reviewed changes

blambov force-pushed the CASSANDRA-18397 branch from 4fdaf1a to f586649 Compare July 19, 2023 12:58

blambov closed this Jul 20, 2023

CASSANDRA-18397: Unified Compaction Strategy #2287

CASSANDRA-18397: Unified Compaction Strategy #2287

Conversation

blambov commented Apr 20, 2023 • edited Loading

Maxwell-Guo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gerrrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gerrrr commented Jun 26, 2023

Maxwell-Guo commented Jun 27, 2023 • edited Loading

blambov commented Jun 28, 2023

blambov commented Jul 18, 2023 • edited Loading

Maxwell-Guo commented Jul 18, 2023

Maxwell-Guo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blambov commented Jul 20, 2023

blambov commented Jul 20, 2023

blambov commented Jul 20, 2023

blambov commented Apr 20, 2023 •

edited

Loading

Maxwell-Guo commented Jun 27, 2023 •

edited

Loading

blambov commented Jul 18, 2023 •

edited

Loading