autorepair v2 framework (against 4.1.6)

[jaydeepkumar1984]

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

[tolbertam]

👍 thanks for doing this. I imagine it could be useful to be able to configure things more granularly. I have some cases where we want to run full repair on a table, but never incremental (the main use case is analytics based where etl processes are loading a bunch of SSTables every few hours). Will come back to this as I review and experiment more with the PR, if there is a bunch of things that we would want configurable at table level maybe we could consider adding something like a RepairParams, otherwise maybe this is fine as is.

[tolbertam]

This may be overkill and some of the options may not be applicable, but wonder if we could just have a single field that functions similarly to AutoRepairConfig.repair_type_overrides:

So one could do something like:

ALTER TABLE x.y WITH automated_repair={'incremental': {'enabled': false}};

Most of the repair_type_overrides aren't really applicable at a table level, so maybe we'd just have a separate class like AutoRepairParams that functioned similarly to CompactionParams

[jaydeepkumar1984]

👍 thanks for doing this. I imagine it could be useful to be able to configure things more granularly. I have some cases where we want to run full repair on a table, but never incremental (the main use case is analytics based where etl processes are loading a bunch of SSTables every few hours). Will come back to this as I review and experiment more with the PR, if there is a bunch of things that we would want configurable at table level maybe we could consider adding something like a RepairParams, otherwise maybe this is fine as is.

Sure, makes sense. We can finalize all the configuration granularity that we want at a table level and then change it accordingly.

[jaydeepkumar1984]

I incorporated this comment. Now, we have more granular configuration support as follows:

    AND automated_repair_full = {'enabled': 'true'}
    AND automated_repair_incremental = {'enabled': 'true'};

Please note: I had to use two separate configurations instead of merging due to the limitation of the CQL grammar, which does not allow nested configuration for DDL statements.

    AND automated_repair = {'full': {'enabled': 'true'}, 'incremental': {'enabled': 'true'}}

[tolbertam]

I think for a first pass this is ok, but eventually would like to see this apply more at a table label; Tables that have/are views would be skipped for incremental, and also those that have CDC enabled.

[jaydeepkumar1984]

Sure, we can provide a more granular table-level configuration with multiple configuration knobs.

[jaydeepkumar1984]

Done - please see above.

[tolbertam]

Nit: there are some existing methods (getInt, getUUID) that are overloaded with an additional ifNull parameter, for consistency we should do the same for getLong.

Suggested change

	// this function will return the default value if the row doesn't have that column or the column data is null
	// This function is used to avoid the nullpointerexception
	public long getLongOrDefault(String column, long defaultValue) {
	if (!has(column)){
	return defaultValue;
	}
	return getLong(column);
	}
	public long getLong(String column, long ifNull) {
	ByteBuffer bytes = data.get(column);
	return bytes == null ? ifNull : LongType.instance.compose(bytes);
	}

[jaydeepkumar1984]

Nit: there are some existing methods (getInt, getUUID) that are overloaded with an additional ifNull parameter, for consistency we should do the same for getLong.

ACK - will update it

[jaydeepkumar1984]

Done

[tolbertam]

Is there precedence for using Lightweight transactions in internal cassandra stuff? Looking at how this is used, I see we check to see if the CAS insert was applied, but that just effects logging, so I think there is little harm in removing IF NOT EXISTS and just let it overwrite.

[tolbertam]

Similar to INSERT_NEW_REPAIR_HISTORY, we can just do a blind delete here (no need for IF EXISTS), if it doesn't exist it'll just create a row tombstone. Given how this is only executed when we detect the host isn't in the ring seems fine to just do that.

[tolbertam]

Fantastic to have these out of the gate 👍. We should add some docs to metrics.adoc to document these.

[jaydeepkumar1984]

Fantastic to have these out of the gate 👍. We should add some docs to metrics.adoc to document these.

ACK. Will extend the documentation

[jaydeepkumar1984]

Incorporated

[jaydeepkumar1984]

Done

[tolbertam]

One thing worth discussing; by default I noticed that if I set:

auto_repair:
  enabled: true

That it doesn't actually run repairs because by default both full and incremental repair are disabled. Is this how we want this to work? I think it's a bit subjective and I don't have a good answer, but felt it was worth discussing.

I think two possible good defaults would be:

1. Keep it as-is, if you want to enable repair, you should have to choose which of the repair options you would want enabled by default.
2. Full repair is enabled with the above config.

I think maybe what we should just do is go with option one and add a commented section in cassandra.yaml that looks something like this:

# Documentation about auto repair goes here
# auto_repair:
#  # Enables auto repair, defaults to false (disabled)
#  enabled: true
#  repair_type_overrides:
#    full:
#      # Enables auto full repairs, defaults to false
#      enabled: true
#     # ... all other various property defaults here
#    incremental:
#      enabled: false

[jaydeepkumar1984]

IMO, by default, we should just enable full repair. For the IR, we should let an operator decide whether they want to enable or not.

[tolbertam]

that seems sensible to me 👍. I'm not sure whether or not this would get too tricky, but I could see that the 'defaults' for overrides could be contextual based on the kind of repair, e.g. I would want a more frequent min_repair_interval_in_hours for Incremental Repair than for Full Repair. I think that may be too involved though, I think it's ok to just special case full repair to be enabled by default if you have auto_repair.enabled: true

[jaydeepkumar1984]

Done. I changed the min_repair_interval_in_hours to DurationSpec.IntSecondsBound so we can now go up to the second level of granularity.

[tolbertam]

Small preference: It could be better to just put this on RepairType, can just define a AutoRepairState getAutoRepairState() on the RepairType enum.

[jaydeepkumar1984]

Done

[tolbertam]

I like the way you did this. Seems like if I wanted to in theory add a new repair type to be scheduled (e.g. 'Preview repairs on repaired set', 'Preview repairs on unrepaired set', 'Paxos repairs'). I'd just have to add a new enum here, and then implement a AutoRepairState for it and add it to the AutoRepairStateFactory? Nice!

[tolbertam]

Seems to be the case! That was incredibly easy!

git diff
diff --git a/src/java/org/apache/cassandra/repair/autorepair/AutoRepairConfig.java b/src/java/org/apache/cassandra/repair/autorepair/AutoRepairConfig.java
index 731d95a8dd..03d3b240c5 100644
--- a/src/java/org/apache/cassandra/repair/autorepair/AutoRepairConfig.java
+++ b/src/java/org/apache/cassandra/repair/autorepair/AutoRepairConfig.java
@@ -46,7 +46,7 @@ public class AutoRepairConfig implements Serializable
     public volatile Options global_settings;
 
     public enum RepairType
-    {full, incremental}
+    {full, incremental, preview_repaired}
 
     // repair_type_overrides overrides the global_settings for a specific repair type
     public volatile Map<RepairType, Options> repair_type_overrides = new EnumMap<>(RepairType.class);
diff --git a/src/java/org/apache/cassandra/repair/autorepair/AutoRepairState.java b/src/java/org/apache/cassandra/repair/autorepair/AutoRepairState.java
index fc0281a7f0..ebda85c52a 100644
--- a/src/java/org/apache/cassandra/repair/autorepair/AutoRepairState.java
+++ b/src/java/org/apache/cassandra/repair/autorepair/AutoRepairState.java
@@ -283,6 +283,27 @@ public abstract class AutoRepairState implements ProgressListener
     }
 }
 
+class PreviewRepairedState extends AutoRepairState
+{
+    public PreviewRepairedState()
+    {
+        super(RepairType.preview_repaired);
+    }
+
+
+    @Override
+    public RepairRunnable getRepairRunnable(String keyspace, List<String> tables, Set<Range<Token>> ranges, boolean primaryRangeOnly)
+    {
+        RepairOption option = new RepairOption(RepairParallelism.PARALLEL, primaryRangeOnly, false, false,
+                                               AutoRepairService.instance.getAutoRepairConfig().getRepairThreads(repairType), ranges,
+                                               !ranges.isEmpty(), false, false, PreviewKind.REPAIRED, false, true, false, false);
+
+        option.getColumnFamilies().addAll(tables);
+
+        return getRepairRunnable(keyspace, option);
+    }
+}
+
 class IncrementalRepairState extends AutoRepairState
 {
     public IncrementalRepairState()
diff --git a/src/java/org/apache/cassandra/repair/autorepair/AutoRepairStateFactory.java b/src/java/org/apache/cassandra/repair/autorepair/AutoRepairStateFactory.java
index 3812ed6e4d..8c3ba373db 100644
--- a/src/java/org/apache/cassandra/repair/autorepair/AutoRepairStateFactory.java
+++ b/src/java/org/apache/cassandra/repair/autorepair/AutoRepairStateFactory.java
@@ -30,6 +30,8 @@ public class AutoRepairStateFactory
                 return new FullRepairState();
             case incremental:
                 return new IncrementalRepairState();
+            case preview_repaired:
+                return new PreviewRepairedState();
         }
 
         throw new IllegalArgumentException("Invalid repair type: " + repairType);

INFO  [Thread-12] 2024-06-20 14:02:32,047 RepairRunnable.java:324 - Starting repair command #11 (a5a3fffa-2f37-11ef-a8c2-e106f7a22c03), repairing keyspace easy_cass_stress with repair options (parallelism: parallel, primary range: true, incremental: false, job threads: 1, ColumnFamilies: [keyvalue], dataCenters: [], hosts: [], previewKind: REPAIRED, # of ranges: 1, pull r
epair: false, force repair: false, optimise streams: false, ignore unreplicated keyspaces: true, repairPaxos: false, paxosOnly: false)

[tolbertam]

One thing I just stumbled into is that the formatting of multi-word config here is different than the yaml configuration, e.g. minrepairinternvalinhours in the yaml is min_repair_internval_in_hours. Can we update the nodetool config to match the yaml one?

[jaydeepkumar1984]

Sure, will do

[jaydeepkumar1984]

Done

[tolbertam]

Suggestion: implement toString() to return all the options like a map, otherwise system_views.settings can't read these:

auto_repair.repair_type_overrides | {incremental=org.apache.cassandra.repair.autorepair.AutoRepairConfig$Options@64af73a1, preview_repaired=org.apache.cassandra.repair.autorepair.AutoRepairConfig$Options@2ae56cae}

[jaydeepkumar1984]

Done

[tolbertam]

Should do something similar we did for other system keyspaces in CassandraRelevantProperties:

    SYSTEM_AUTH_DEFAULT_RF("cassandra.system_auth.default_rf", "1"),
    SYSTEM_TRACES_DEFAULT_RF("cassandra.system_traces.default_rf", "2"),
    SYSTEM_DISTRIBUTED_DEFAULT_RF("cassandra.system_distributed.default_rf", "3"),

which is used like:

KeyspaceParams.simple(Math.max(DEFAULT_RF, DatabaseDescriptor.getDefaultKeyspaceRF()))

[tolbertam]

although, i wonder if we should just leverage system_distributed instead of creating a separate keyspace? This may reduce some operator burden. e.g. in our cluster bring up one of the first things we do is alter these keyspaces to network topology strategy and we also have allowlist/blocklist for system keyspaces that we'd have to update.

[jaydeepkumar1984]

Done. Moved these two tables under system_distributed itself.

[tolbertam]

Discussion point: We are currently passing false for optimiseStreams. This is a nice enhancement in the form of CASSANDRA-3200 which cuts down the amount of overstreaming when inconsistencies are found.

I'm not sure it has a lot of precedence for use, but it's been in tree for a long time. We've only started to look into rolling it out ourselves.

I think we should consider setting it to true here, or making it configurable somehow.

[jaydeepkumar1984]

Ok, changed it to true

[clohfink]

NIT: from a naming, having AUTOMATED_REPAIR: true vs DISABLED_AUTOMATED_REPAIR: false is easier for people to reason. Avoiding the double negatives and extra boolean logic for what people expect.

[jaydeepkumar1984]

ACK - will update it accordingly

[jaydeepkumar1984]

Done

[clohfink]

Since this is a global setting, I am not sure this is safe or necessary. Other keyspaces not in an island configuration to a single DC will not get repaired correctly (e.g., system_distributed, system_auth). For single keyspaces that are not replicated, there are already checks on a node if it is not replicated for a keyspace in checkNodeContainsKeyspaceReplica of AutoRepairUtils. Could simplify things a little bit and get rid of the pid in schema?

[jaydeepkumar1984]

Yeah, it is not necessary. I have removed it.

[clohfink]

this is kinda across all of these settings, but if look at Config.java . Can see now after 4.x that we for time settings instead of having _in_sec just pass a duration type. Same goes for using the mib options intead of _mb etc. Should probably stick with this kinda thing for most of the config options

[jaydeepkumar1984]

Updated the configs to match with the 4.x by using the DurationSpec.*