CEP-37 on Trunk by jaydeepkumar1984 · Pull Request #3598 · apache/cassandra

jaydeepkumar1984 · 2024-10-03T20:41:44Z

CEP:
https://cwiki.apache.org/confluence/display/CASSANDRA/CEP-37+Apache+Cassandra+Unified+Repair+Solution

Design doc:
https://docs.google.com/document/d/1CJWxjEi-mBABPMZ3VWJ9w5KavWfJETAGxfUpsViPcPo/edit?tab=t.0#heading=h.r112r46toau0

The Cassandra JIRA: https://issues.apache.org/jira/browse/CASSANDRA-19918

Code review comments

The comments mentioned in this PR are not exhaustive; a lot of review happened on 4.1 PR, so please refer to those comments as well here: autorepair v2 framework (against 4.1.6) #3367
There have been significant code reviews happend on individual sub-PRs, some of them are here:

AutoRepair documentation improvements: AutoRepair documentation improvements jaydeepkumar1984/cassandra#22
Implement preview_repaired auto-repair type: Implement preview_repaired auto-repair type jaydeepkumar1984/cassandra#4
RangeRepairSplitter prototype: RangeRepairSplitter prototype jaydeepkumar1984/cassandra#3
Make RepairTokenRangeSplitter the default implementation: Make RepairTokenRangeSplitter the default implementation jaydeepkumar1984/cassandra#18
Update RepairTokenRangeSplitter to work with BTI-formatted SSTables Update RepairTokenRangeSplitter to work with BTI-formatted SSTables jaydeepkumar1984/cassandra#36
Deprioritize repair on nodes whose replicas are actively being repaired Avoid returning MY_TURN when replicas are actively taking their turn jaydeepkumar1984/cassandra#51
Update the documentation Auto Repair Documentation improvements, bytes_per_assignment to 50GiB jaydeepkumar1984/cassandra#47
Avoid micro splitting of token ranges, specifically with vnodes Avoid micros splitting of token ranges for FixedSplitTokenRangeSplitter jaydeepkumar1984/cassandra#46
Avoid selecting nodes when their replicas are already scheduling repairs https://github.com/jaydeepkumar1984/cassandra/pull/51/files

michaelsembwever · 2024-10-19T15:23:30Z

@masokol , @emolsson , @itskarlsson , @tommystendahl , @etedpet , @jwaeab , @VictorCavichioli , @SajidRiaz138 , @ch1bbe , @ArcturusMengsk , @DanielwEriksson , @manmagic3

as contributors to https://github.com/Ericsson/ecchronos we would very much appreciate any last-minute-pre-CEP-vote review to this PR for Cassandra's new repair solution. (more technical/code review will continue post-CEP vote.)

CEP:
https://cwiki.apache.org/confluence/display/CASSANDRA/CEP-37+Apache+Cassandra+Unified+Repair+Solution

Design doc:
https://docs.google.com/document/d/1CJWxjEi-mBABPMZ3VWJ9w5KavWfJETAGxfUpsViPcPo/edit?tab=t.0#heading=h.r112r46toau0

masokol · 2024-10-21T08:12:12Z

Hi,

I think this looks promising! I have a few points:

Combine ranges instead of splitting - in ecChronos we saw huge improvements in some scenarios (when the data is low or empty tables) compared to repairing 1 vnode at a time. This improvement scaled with amount of vnodes, although it might've been related to overhead due to running repairs through JMX. Not sure but might be worth investigating.
Major versions, during major version upgrades like 3 -> 4 we weren't supposed to run repairs. If Cassandra plans to keep this then it would be nice for repairs to automatically pause during major version upgrades.
Observabliity, i saw there're metrics but it would also be nice to see repair status with nodetool.
Repair priority per table, not per node.

jaydeepkumar1984 · 2024-10-21T16:26:42Z

Thanks, @masokol, for the review! Please find my response here:

Combine ranges instead of splitting - in ecChronos we saw huge improvements in some scenarios (when the data is low or empty tables) compared to repairing 1 vnode at a time. This improvement scaled with amount of vnodes, although it might've been related to overhead due to running repairs through JMX. Not sure but might be worth investigating.

Generally, empty tables or tables with a small amount of data runs through pretty fast, in seconds, so it is not a major issue for smaller/empty tables.
The current framework already has support to combine ranges through a setting (src/java/org/apache/cassandra/repair/autorepair/AutoRepair.java:266)

Major versions, during major version upgrades like 3 -> 4 we weren't supposed to run repairs. If Cassandra plans to keep this then it would be nice for repairs to automatically pause during major version upgrades.

This is a great suggestion, and the framework can be enhanced easily - I just filed a new sub-ticket CASSANDRA-20013 to track this

Observabliity, i saw there're metrics but it would also be nice to see repair status with nodetool.

There is already a new nodetool command that would print the current status. Please take a look at it here.

Repair priority per table, not per node.

Currently, it will repair the tables randomly, but it can be enhanced to add a priority as a CQL table property that an end user can configure, which can also be enhanced easily. Just added this enhancement to the ticket mentioned above.

tolbertam

still in process of reviewing but thought I'd leave comments as I go since I still have a bit left.

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

src/java/org/apache/cassandra/service/ActiveRepairService.java

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

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

test/unit/org/apache/cassandra/service/AutoRepairServiceSetterTest.java

src/java/org/apache/cassandra/service/StorageService.java

src/java/org/apache/cassandra/tcm/sequences/BootstrapAndJoin.java

src/java/org/apache/cassandra/utils/FBUtilities.java

test/unit/org/apache/cassandra/config/YamlConfigurationLoaderTest.java

jaydeepkumar1984 · 2025-02-11T04:37:03Z

src/java/org/apache/cassandra/schema/TableParams.java

-                            .readRepair(params.readRepair);
+                            .readRepair(params.readRepair)
+                            .automatedRepair(params.autoRepair)
+        ;


nit: move ";" on the line above

jaydeepkumar1984 · 2025-02-11T04:41:58Z

test/unit/org/apache/cassandra/repair/autorepair/AutoRepairConfigRepairTypeTest.java

+ */
+
+package org.apache.cassandra.repair.autorepair;
+


nit: remove extra line

jaydeepkumar1984 · 2025-02-11T19:03:07Z

src/java/org/apache/cassandra/metrics/AutoRepairMetrics.java

+            }
+        });
+
+


nit: extra line

src/java/org/apache/cassandra/schema/AutoRepairParams.java

jaydeepkumar1984 · 2025-02-11T21:14:51Z

src/java/org/apache/cassandra/tools/NodeProbe.java

+        autoRepairProxy.setAutoRepairEnabled(repairType, enabled);
+    }
+
+    public void setRepairThreads(String repairType, int repairThreads)


s/setRepairThreads/setAutoRepairThreads/ here and elsewhere in this class

jaydeepkumar1984 · 2025-02-12T00:39:47Z

src/java/org/apache/cassandra/metrics/AutoRepairMetrics.java

+public class AutoRepairMetrics
+{
+    public static final String TYPE_NAME = "autorepair";
+    public Gauge<Integer> repairsInProgress;


This and the other Gauge in this class can be final

jaydeepkumar1984 · 2025-02-12T00:52:28Z

src/java/org/apache/cassandra/repair/autorepair/AutoRepairConfig.java

+            opts.table_max_repair_time = new DurationSpec.IntSecondsBound("6h");
+            opts.materialized_view_repair_enabled = false;
+            opts.token_range_splitter = new ParameterizedClass(DEFAULT_SPLITTER.getName(), Collections.emptyMap());
+            opts.initial_scheduler_delay = new DurationSpec.IntSecondsBound("5m"); // 5 minutes


The comment is not needed here

jaydeepkumar1984 · 2025-02-12T00:56:44Z

src/java/org/apache/cassandra/repair/autorepair/AutoRepairUtils.java

+    COL_REPAIR_TYPE, COL_HOST_ID);
+
+    final static String RECORD_FINISH_REPAIR_HISTORY = String.format(
+


nit: remove empty line

jaydeepkumar1984 · 2025-02-12T04:35:50Z

src/java/org/apache/cassandra/schema/AutoRepairParams.java

+        }
+    }
+
+    private ImmutableMap<String, String> options;


Make this a final.

jaydeepkumar1984 · 2025-02-12T04:36:53Z

src/java/org/apache/cassandra/schema/AutoRepairParams.java

+        return StringUtils.isNumeric(value);
+    }
+
+


nit: remove extra line

jaydeepkumar1984 · 2025-02-12T04:37:53Z

src/java/org/apache/cassandra/schema/SchemaKeyspace.java

-               .add("extensions", params.extensions);
+               .add("extensions", params.extensions)
+               .add("auto_repair", params.autoRepair.asMap());
+


nit: remove the new line

jaydeepkumar1984 · 2025-02-12T04:38:34Z

src/java/org/apache/cassandra/schema/SchemaKeyspace.java

-                                                 .readRepair(getReadRepairStrategy(row));
+                                                 .readRepair(getReadRepairStrategy(row))
+                                                 .automatedRepair(AutoRepairParams.fromMap(row.getFrozenTextMap("auto_repair")))
+;


move it to the above line

jaydeepkumar1984 · 2025-02-12T04:41:46Z

src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java

+    private static final TableMetadata AutoRepairPriorityTable =
+            parse(AUTO_REPAIR_PRIORITY, "Auto repair priority for each group", AUTO_REPAIR_PRIORITY_CQL).build();
+
+


nit: remove extra line

jaydeepkumar1984 · 2025-02-12T04:42:39Z

src/java/org/apache/cassandra/schema/TableParams.java

-        READ_REPAIR;
+        READ_REPAIR,
+        AUTO_REPAIR,
+        ;


move it above

jaydeepkumar1984 · 2025-02-12T04:46:37Z

src/java/org/apache/cassandra/service/AutoRepairServiceMBean.java

+public interface AutoRepairServiceMBean
+{
+    /**
+     * Enable or disable auto-repair for a given repair type


this comment is not needed as APIs are self explanatory

…geSplitter.java Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

Previously AutoRepairUtils.myTurnToRunRepair would return MY_TURN if there is capacity to select a new node for repair and the current node has the oldest lastRepairFinishTime. This change adjusts this method to make use of a newly added method getMostEligibleHostToRepair which will identify the node with the oldest lastRepairFinishTime of which none of the node's replicas are actively repairing. This should prevent replicas from actively repairing concurrently, which would increase load on replica sets and also create anticompaction conflicts for incremental repair. Behavior is configurable by allow_parallel_replica_repair and allow_parallel_replica_repair_across_schedules configuration. patch by Andy Tolbert, reviewed by Jaydeepkumar Chovatia and Francisco Guerrero for CASSANDRA-20180

jaydeepkumar1984 · 2025-04-20T17:40:31Z

Rebase on top of the latest trunk has been completed; all the unit tests are passing on CircleCI

Many dtests are failing on CircleCI due to resource limitations. For that, I have run them internally inside our infrastructure and most of them are passing as expected.

frankgh

Great work! I left a few minor comments. Also I have a few NITs , so feel free to ignore those

frankgh · 2025-04-20T22:43:45Z

src/java/org/apache/cassandra/service/ActiveRepairService.java

+
+    public void setIncrementalRepairDiskHeadroomRejectRatio(double value)
+    {
+        DatabaseDescriptor.setIncrementalRepairDiskHeadroomRejectRatio(value);


can we validate for only positive numbers to be set here between the values of 0-1?

frankgh · 2025-04-20T22:45:17Z

src/java/org/apache/cassandra/schema/SystemDistributedKeyspace.java

     * // TODO: TCM - how do we evolve these tables?
     */
-    public static final long GENERATION = 6;
+    public static final long GENERATION = 7;


can we add a description for gen 7 above?

doc/modules/cassandra/pages/managing/operating/metrics.adoc

frankgh · 2025-04-21T17:31:21Z

src/java/org/apache/cassandra/repair/autorepair/RepairTokenRangeSplitter.java

+ * <p>
+ * While this splitter has a lot of tuning parameters, the expectation is that the established default configuration
+ * shall be sensible for all {@link org.apache.cassandra.repair.autorepair.AutoRepairConfig.RepairType}'s. The following
+ * configuration parameters are offered.


did we intend to list the configuration parameters below in the javadocs?

Incorporated

src/java/org/apache/cassandra/repair/autorepair/RepairTokenRangeSplitter.java

src/java/org/apache/cassandra/schema/AutoRepairParams.java

Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

…geSplitter.java Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

frankgh

+1

…0572 Validate disk headroom for IR and a couple of comment updates

Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

jaydeepkumar1984 force-pushed the trunk_cep_37 branch 2 times, most recently from ee6b375 to 2f42d8c Compare October 4, 2024 19:06

jaydeepkumar1984 changed the title ~~[Draft] CEP-37 on Trunk~~ CEP-37 on Trunk Oct 8, 2024

jaydeepkumar1984 force-pushed the trunk_cep_37 branch from 75b0bad to a145408 Compare October 9, 2024 03:39

jaydeepkumar1984 force-pushed the trunk_cep_37 branch from db7d73b to d3589c2 Compare October 17, 2024 23:50

jaydeepkumar1984 force-pushed the trunk_cep_37 branch from d3589c2 to 3610701 Compare October 22, 2024 20:31

jaydeepkumar1984 force-pushed the trunk_cep_37 branch 2 times, most recently from 5730aa2 to 126684a Compare November 6, 2024 01:47

jaydeepkumar1984 force-pushed the trunk_cep_37 branch 2 times, most recently from ec99162 to f9f6971 Compare November 21, 2024 05:20

jaydeepkumar1984 force-pushed the trunk_cep_37 branch from 95d0a5e to e093b34 Compare January 29, 2025 19:06

tolbertam reviewed Feb 10, 2025

View reviewed changes

jaydeepkumar1984 commented Feb 11, 2025

View reviewed changes

src/java/org/apache/cassandra/metrics/AutoRepairMetrics.java Outdated

}

});

Copy link

Contributor Author

jaydeepkumar1984 Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line

jaydeepkumar1984 commented Feb 11, 2025

View reviewed changes

src/java/org/apache/cassandra/schema/AutoRepairParams.java Show resolved Hide resolved

jaydeepkumar1984 commented Feb 11, 2025

View reviewed changes

jaydeepkumar1984 commented Feb 12, 2025

View reviewed changes

tolbertam and others added 12 commits April 18, 2025 09:06

Doc and conf fixes, initial IR enablement guidance

706b390

Update src/java/org/apache/cassandra/repair/autorepair/RepairTokenRan…

b809c4a

…geSplitter.java Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

Update src/java/org/apache/cassandra/repair/autorepair/RepairTokenRan…

5c58c2e

…geSplitter.java Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

Update num_of_subranges

45edb04

Move auto-repair retry policy config to repair-type level

317dfbb

Update docs

1af049a

Add toString to AutoRepairConfig.java

f7c862d

Create AutoRepair executors only if enabled and a few other nits

d6b7a46

parens in next line

729b0d3

Adjust based on the latest trunk

beb8b61

Merge branch 'trunk_cep_37_rebase_on_apr_17_2025' into trunk_cep_37

89b4ab6

jaydeepkumar1984 force-pushed the trunk_cep_37 branch from 9feb931 to 89b4ab6 Compare April 19, 2025 22:01

Merge branch 'trunk' into trunk_cep_37

ab6fe8e

frankgh reviewed Apr 21, 2025

View reviewed changes

jaydeepkumar1984 and others added 7 commits April 21, 2025 15:03

Update doc/modules/cassandra/pages/managing/operating/metrics.adoc

9e9c035

Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

Update src/java/org/apache/cassandra/repair/autorepair/RepairTokenRan…

eff68db

…geSplitter.java Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

Update src/java/org/apache/cassandra/schema/AutoRepairParams.java

44a1694

Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

Update src/java/org/apache/cassandra/schema/AutoRepairParams.java

a957d90

Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

Update src/java/org/apache/cassandra/schema/AutoRepairParams.java

0abf12a

Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

Validate disk headroom for IR and a couple of comment updates

5f4a938

Format { on the new line

ab6c770

frankgh approved these changes Apr 21, 2025

View reviewed changes

jaydeepkumar1984 and others added 2 commits April 21, 2025 15:49

Merge pull request #63 from jaydeepkumar1984/trunk_cep_37_cassandra_2…

6649d27

…0572 Validate disk headroom for IR and a couple of comment updates

Update doc/modules/cassandra/pages/managing/operating/metrics.adoc

34bf406

Co-authored-by: Francisco Guerrero <frank.guerrero@gmail.com>

clohfink approved these changes Apr 22, 2025

View reviewed changes

belliottsmith force-pushed the trunk branch 2 times, most recently from df3eb40 to 54e39a9 Compare July 23, 2025 11:19

jaydeepkumar1984 closed this Sep 24, 2025

		COL_REPAIR_TYPE, COL_HOST_ID);

		final static String RECORD_FINISH_REPAIR_HISTORY = String.format(

		private static final TableMetadata AutoRepairPriorityTable =
		parse(AUTO_REPAIR_PRIORITY, "Auto repair priority for each group", AUTO_REPAIR_PRIORITY_CQL).build();

		*/

		package org.apache.cassandra.repair.autorepair;

Conversation

jaydeepkumar1984 commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelsembwever commented Oct 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

masokol commented Oct 21, 2024

Uh oh!

jaydeepkumar1984 commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tolbertam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaydeepkumar1984 Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaydeepkumar1984 commented Apr 20, 2025

Uh oh!

frankgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frankgh left a comment

Choose a reason for hiding this comment

Uh oh!

jaydeepkumar1984 commented Oct 3, 2024 •

edited

Loading

michaelsembwever commented Oct 19, 2024 •

edited

Loading

jaydeepkumar1984 commented Oct 21, 2024 •

edited

Loading

jaydeepkumar1984 Feb 12, 2025 •

edited

Loading