You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "himanshujindal (via GitHub)" <gi...@apache.org> on 2025/08/12 22:40:24 UTC
[PR] Stop repair scheduler if two major versions are detected [cassandra]
himanshujindal opened a new pull request, #4304:
URL: https://github.com/apache/cassandra/pull/4304
Do not allow repair scheduler to run repair if two major versions are detected. This can happen during major version upgrade, and we don't want repair to run since the streaming data for repair can be incompatible across major versions.
Users can override this using a cassandra yaml config if they want to.
Also, this does not affect minor version upgrades, or repair jobs run manually.
https://issues.apache.org/jira/browse/CASSANDRA-20048
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2301719313
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -5940,6 +5940,16 @@ public static void setIncrementalRepairDiskHeadroomRejectRatio(double value)
conf.incremental_repair_disk_headroom_reject_ratio = value;
}
+ public static boolean getMixedMajorVersionRepairEnabled()
+ {
+ return conf.auto_repair.mixed_major_version_repair_enabled;
+ }
+
+ public static void setMixedMajorVersionRepairEnabled(boolean mixed_major_version_repair_enabled)
+ {
+ conf.auto_repair.mixed_major_version_repair_enabled = mixed_major_version_repair_enabled;
+ }
+
Review Comment:
Thanks for pointing me to to that tool. Updated that tool to read this config as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2305574220
##########
src/java/org/apache/cassandra/tools/nodetool/SetAutoRepairConfig.java:
##########
@@ -176,6 +176,9 @@ public void execute(NodeProbe probe)
case "repair_retry_backoff":
probe.setAutoRepairRetryBackoff(repairTypeStr, paramVal);
break;
+ case "mixed_major_version_repair_enabled":
Review Comment:
Can we add the output to `getautorepairconfig` as well? Looks like setting this works, but would be nice to verify that it works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2296375066
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -5940,6 +5940,16 @@ public static void setIncrementalRepairDiskHeadroomRejectRatio(double value)
conf.incremental_repair_disk_headroom_reject_ratio = value;
}
+ public static boolean getMixedMajorVersionRepairEnabled()
+ {
+ return conf.auto_repair.mixed_major_version_repair_enabled;
+ }
+
+ public static void setMixedMajorVersionRepairEnabled(boolean mixed_major_version_repair_enabled)
+ {
+ conf.auto_repair.mixed_major_version_repair_enabled = mixed_major_version_repair_enabled;
+ }
+
Review Comment:
It looks like we already have a `AutoRepairConfig.isMixedMajorVersionRepairEnabled()` which is currently unused. Since this is an `AutoRepair`-specific thing can we remove these methods and use methods on `AutoRepairConfig` instead?
Would also be nice to update `SetAutoRepairConfig` to allow configuring this value.
##########
test/unit/org/apache/cassandra/repair/autorepair/AutoRepairUtilsTest.java:
##########
@@ -232,6 +236,52 @@ public void testGetHostIdsInCurrentRing_multiple_nodes()
assertTrue(hosts.contains(hostId));
}
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithSingleNode()
+ {
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameVersion()
+ {
+ ClusterMetadataTestHelper.addEndpoint(2);
+ // Test the current behavior with the existing cluster setup
+ // In a single-node test environment, this should return false
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameMajorVersionDifferentMinorVersions()
+ {
+ // add two nodes with cassandra 5 major version, but different minor version
+ CassandraVersion differentCassandraVersion = new CassandraVersion(
+ String.format("%d.%d",
+ NodeVersion.CURRENT.cassandraVersion.major,
+ NodeVersion.CURRENT.cassandraVersion.minor+1));
+ ClusterMetadataTestHelper.addEndpoint(2, new NodeVersion(
+ differentCassandraVersion,
+ NodeVersion.CURRENT_METADATA_VERSION));
+ // With the same major versions, but different minor versions, we should still see this function return true
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfDifferentMajorVersions()
+ {
+ // add two nodes with cassandra 5 major version, but different minor version
Review Comment:
A bit pedantic I know, but in future the current major version will change.
```suggestion
// add two nodes with current cassandra major version, but different minor version
```
##########
conf/cassandra.yaml:
##########
@@ -2779,6 +2779,9 @@ storage_compatibility_mode: NONE
# # for a specified duration to ensure they are indeed removed before adjustments are made to the schedule.
# history_clear_delete_hosts_buffer_interval: 2h
# # NOTE: Each of the below settings can be overridden per repair type under repair_type_overrides
+# # by default repair is disabled if there are mixed major versions detected - which would happen
+# # if a major version upgrade is being performed on the cluster, but a user can enable it using this flag
+# # mixed_major_version_repair_enabled = false;
Review Comment:
Small suggestions:
* Move `#NOTE: ...` to the line before `global_settings` as it makes more sense to be proximate to there.
* Uncomment `mixed_major_version_repair_enabled = false` and remove semi-colon.
```suggestion
# # by default repair is disabled if there are mixed major versions detected - which would happen
# # if a major version upgrade is being performed on the cluster, but a user can enable it using this flag
# mixed_major_version_repair_enabled = false
# # NOTE: Each of the below settings can be overridden per repair type under repair_type_overrides
```
A couple of other things that would to be good to do:
1. Can you also add this to `cassandra_latest.yaml` as well?
2. Add to 'Top level settings' in auto_repair.adoc? In that file it would also be good to expand on why repairs should not be run during major version upgrades (previous slack thread about it: https://the-asf.slack.com/archives/C06MR8K2Q0Z/p1729716524563759)
##########
test/distributed/org/apache/cassandra/distributed/test/log/ClusterMetadataTestHelper.java:
##########
@@ -798,19 +798,27 @@ public MoveProcess finishMove()
public static void addEndpoint(int i)
{
+ addEndpoint(i, NodeVersion.CURRENT);
+ }
+
+ public static void addEndpoint(int i, NodeVersion nodeVersion) {
try
Review Comment:
`{` on next line
```suggestion
public static void addEndpoint(int i, NodeVersion nodeVersion)
{
try
```
##########
test/distributed/org/apache/cassandra/distributed/test/log/ClusterMetadataTestHelper.java:
##########
@@ -798,19 +798,27 @@ public MoveProcess finishMove()
public static void addEndpoint(int i)
{
+ addEndpoint(i, NodeVersion.CURRENT);
+ }
+
+ public static void addEndpoint(int i, NodeVersion nodeVersion) {
try
{
- addEndpoint(InetAddressAndPort.getByName("127.0.0." + i), new Murmur3Partitioner.LongToken(i));
+ addEndpoint(InetAddressAndPort.getByName("127.0.0." + i), new Murmur3Partitioner.LongToken(i), nodeVersion);
}
catch (UnknownHostException e)
{
throw new RuntimeException(e);
}
}
- public static void addEndpoint(InetAddressAndPort endpoint, Token t)
+ public static void addEndpoint(InetAddressAndPort endpoint, Token t) {
+ addEndpoint(endpoint, t, NodeVersion.CURRENT);
Review Comment:
{ on next line:
```suggestion
public static void addEndpoint(InetAddressAndPort endpoint, Token t)
{
addEndpoint(endpoint, t, NodeVersion.CURRENT);
```
##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepairUtils.java:
##########
@@ -425,6 +425,21 @@ public static CurrentRepairStatus getCurrentRepairStatus(RepairType repairType,
return null;
}
+ /**
+ * Checks whether the cluster has multiple major versions
+ * @return
+ * true if more than one major versions are detected
+ * false if only one major version is detected
+ *
+ */
+ public static boolean hasMultipleLiveMajorVersions()
+ {
+ ClusterMetadata metadata = ClusterMetadata.current();
+ int maxMajorVersion = ClusterMetadata.current().directory.clusterMaxVersion.cassandraVersion.major;
+ int minMajorVersion = ClusterMetadata.current().directory.clusterMinVersion.cassandraVersion.major;
+ return maxMajorVersion != minMajorVersion;
+ }
+
Review Comment:
👍 nice and simple.
I had some doubts on whether this is sufficient given Cassandra's version history. e.g. between 2.0 and 2.1 maybe this wouldn't be safe? But I'd hope from C* 6.0 on this is probably sufficient.
##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepair.java:
##########
@@ -165,6 +166,11 @@ public void repair(AutoRepairConfig.RepairType repairType)
logger.debug("Auto-repair is disabled for repair type {}", repairType);
return;
}
+ if (!DatabaseDescriptor.getMixedMajorVersionRepairEnabled() &&
+ Gossiper.instance.hasMultipleLiveMajorVersions()) {
+ logger.info("Auto-repair is disabled when nodes in the cluster have different major versions");
Review Comment:
I think the metrics are calculated off of / engaged by `AutoRepairState` (accessed 2 lines below). You could add something to that. Curious on the metric we are interested in adding? I'm guess maybe a `Counter` that increments whenever repair is skipped because mixed versions are detected?
##########
test/unit/org/apache/cassandra/repair/autorepair/AutoRepairUtilsTest.java:
##########
@@ -232,6 +236,52 @@ public void testGetHostIdsInCurrentRing_multiple_nodes()
assertTrue(hosts.contains(hostId));
}
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithSingleNode()
+ {
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameVersion()
+ {
+ ClusterMetadataTestHelper.addEndpoint(2);
+ // Test the current behavior with the existing cluster setup
+ // In a single-node test environment, this should return false
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameMajorVersionDifferentMinorVersions()
+ {
+ // add two nodes with cassandra 5 major version, but different minor version
Review Comment:
```suggestion
// add two nodes with current cassandra major version, but different minor version
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2307751315
##########
src/java/org/apache/cassandra/service/AutoRepairService.java:
##########
@@ -102,6 +102,7 @@ public String getAutoRepairConfiguration()
appendConfig(sb, "repair_check_interval", config.getRepairCheckInterval());
appendConfig(sb, "repair_task_min_duration", config.getRepairTaskMinDuration());
appendConfig(sb, "history_clear_delete_hosts_buffer_interval", config.getAutoRepairHistoryClearDeleteHostsBufferInterval());
+ appendConfig(sb, "mixed", config.getMixedMajorVersionRepairEnabled());
Review Comment:
Should have the full name here: `mixed_major_version_repair_enabled`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal closed pull request #4304: Stop repair scheduler if two major versions are detected
URL: https://github.com/apache/cassandra/pull/4304
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2305578120
##########
doc/modules/cassandra/pages/managing/operating/auto_repair.adoc:
##########
@@ -219,6 +219,10 @@ in nodetool repair. General advice is to keep this true.
| repair_retry_backoff | 30s | Backoff time before retrying a repair session.
| repair_task_min_duration | 5s | Minimum duration for the execution of a single repair task. This prevents the
scheduler from overwhelming the node by scheduling too many repair tasks in a short period of time.
+| mixed_major_version_repair_enabled | false | Enable/Disable running repairs on the cluster when there are mixed
+major versions detected, which usually occurs when the cluster is being upgraded. Repairs between nodes of
+different major versions is not something that is tested, so this may lead to data compatibility issues.
+It is strongly discouraged to set this to true without doing extensive testing beforehand.
Review Comment:
Just realized, since this is not a repair-type level setting, it should be moved up to 'Top level settings' right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2275016374
##########
test/distributed/org/apache/cassandra/distributed/test/GossipTest.java:
##########
@@ -285,4 +288,40 @@ public void testQuarantine() throws IOException
assertTrue(removed);
}
}
+
+ @Test
+ public void testMultipleMajorVersionsWithMultipleMajorVersions() throws Exception
+ {
+ try (Cluster cluster = builder()
+ .withNodes(2)
+ .withConfig(config -> config.with(GOSSIP))
+ .start())
+ {
+ InetSocketAddress peer1 = cluster.get(1).broadcastAddress();
+ String version1 = "4.0.1920";
Review Comment:
Yes, added in new revision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2301737992
##########
test/unit/org/apache/cassandra/repair/autorepair/AutoRepairUtilsTest.java:
##########
@@ -232,6 +236,52 @@ public void testGetHostIdsInCurrentRing_multiple_nodes()
assertTrue(hosts.contains(hostId));
}
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithSingleNode()
+ {
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameVersion()
+ {
+ ClusterMetadataTestHelper.addEndpoint(2);
+ // Test the current behavior with the existing cluster setup
+ // In a single-node test environment, this should return false
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameMajorVersionDifferentMinorVersions()
+ {
+ // add two nodes with cassandra 5 major version, but different minor version
Review Comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2307660874
##########
test/unit/org/apache/cassandra/tools/nodetool/SetAutoRepairConfigTest.java:
##########
@@ -268,7 +268,8 @@ public static Collection<Object[]> testCases()
forEachRepairType("ignore_dcs", "dc1,dc2", (type) -> verify(probe, times(1)).setAutoRepairIgnoreDCs(type.name(), ImmutableSet.of("dc1", "dc2"))),
forEachRepairType("token_range_splitter.max_bytes_per_schedule", "500GiB", (type) -> verify(probe, times(1)).setAutoRepairTokenRangeSplitterParameter(type.name(), "max_bytes_per_schedule", "500GiB")),
forEachRepairType("repair_max_retries", "3", (type) -> verify(probe, times(1)).setAutoRepairMaxRetriesCount(type.name(), 3)),
- forEachRepairType("repair_retry_backoff", "60s", (type) -> verify(probe, times(1)).setAutoRepairRetryBackoff(type.name(), "60s"))
+ forEachRepairType("repair_retry_backoff", "60s", (type) -> verify(probe, times(1)).setAutoRepairRetryBackoff(type.name(), "60s")),
+ forEachRepairType("mixed_major_version_repair_enabled", "false", (type) -> verify(probe, times(1)).setMixedMajorVersionRepairEnabled(false))
Review Comment:
Since this isn't by repair type, it can probably just be in its own test like `testHistoryDeleteHostsClearBufferInSec`
##########
test/unit/org/apache/cassandra/tools/nodetool/SetAutoRepairConfigTest.java:
##########
@@ -268,7 +268,8 @@ public static Collection<Object[]> testCases()
forEachRepairType("ignore_dcs", "dc1,dc2", (type) -> verify(probe, times(1)).setAutoRepairIgnoreDCs(type.name(), ImmutableSet.of("dc1", "dc2"))),
forEachRepairType("token_range_splitter.max_bytes_per_schedule", "500GiB", (type) -> verify(probe, times(1)).setAutoRepairTokenRangeSplitterParameter(type.name(), "max_bytes_per_schedule", "500GiB")),
forEachRepairType("repair_max_retries", "3", (type) -> verify(probe, times(1)).setAutoRepairMaxRetriesCount(type.name(), 3)),
- forEachRepairType("repair_retry_backoff", "60s", (type) -> verify(probe, times(1)).setAutoRepairRetryBackoff(type.name(), "60s"))
+ forEachRepairType("repair_retry_backoff", "60s", (type) -> verify(probe, times(1)).setAutoRepairRetryBackoff(type.name(), "60s")),
+ forEachRepairType("mixed_major_version_repair_enabled", "false", (type) -> verify(probe, times(1)).setMixedMajorVersionRepairEnabled(false))
Review Comment:
Since this isn't by repair type now, it can probably just be in its own test like `testHistoryDeleteHostsClearBufferInSec`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2314240069
##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepair.java:
##########
@@ -165,6 +166,11 @@ public void repair(AutoRepairConfig.RepairType repairType)
logger.debug("Auto-repair is disabled for repair type {}", repairType);
return;
}
+ if (!DatabaseDescriptor.getMixedMajorVersionRepairEnabled() &&
+ Gossiper.instance.hasMultipleLiveMajorVersions()) {
+ logger.info("Auto-repair is disabled when nodes in the cluster have different major versions");
Review Comment:
@jaydeepkumar1984 See my comment here - https://github.com/apache/cassandra/pull/4304#issuecomment-3226130821 and Andy's reply in slack - https://the-asf.slack.com/archives/C06MR8K2Q0Z/p1756306978752409?thread_ts=1755038748.234549&cid=C06MR8K2Q0Z
We decided not to add a metric since logging should be sufficient here. Having said that, I will be open to adding a metric if you align with my proposal presented in the comment linked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2314244827
##########
test/unit/org/apache/cassandra/repair/autorepair/AutoRepairUtilsTest.java:
##########
@@ -232,6 +236,52 @@ public void testGetHostIdsInCurrentRing_multiple_nodes()
assertTrue(hosts.contains(hostId));
}
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithSingleNode()
+ {
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameVersion()
+ {
+ ClusterMetadataTestHelper.addEndpoint(2);
+ // Test the current behavior with the existing cluster setup
+ // In a single-node test environment, this should return false
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameMajorVersionDifferentMinorVersions()
+ {
+ // add two nodes with the current cassandra major version, but different minor version
+ CassandraVersion differentCassandraVersion = new CassandraVersion(
+ String.format("%d.%d",
+ NodeVersion.CURRENT.cassandraVersion.major,
+ NodeVersion.CURRENT.cassandraVersion.minor+1));
+ ClusterMetadataTestHelper.addEndpoint(2, new NodeVersion(
+ differentCassandraVersion,
+ NodeVersion.CURRENT_METADATA_VERSION));
+ // With the same major versions, but different minor versions, we should still see this function return true
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfDifferentMajorVersions()
+ {
+ // add two nodes with different cassandra major versions
+ CassandraVersion differentCassandraVersion = new CassandraVersion(
+ String.format("%d.%d", NodeVersion.CURRENT.cassandraVersion.major - 1, 0));
+ ClusterMetadataTestHelper.addEndpoint(2, new NodeVersion(differentCassandraVersion,
+ NodeVersion.CURRENT_METADATA_VERSION));
+ // With the same major versions, but different minor versions, we should still see this function return true
Review Comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "jaydeepkumar1984 (via GitHub)" <gi...@apache.org>.
jaydeepkumar1984 commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2283814017
##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepair.java:
##########
@@ -165,6 +166,11 @@ public void repair(AutoRepairConfig.RepairType repairType)
logger.debug("Auto-repair is disabled for repair type {}", repairType);
return;
}
+ if (!DatabaseDescriptor.getMixedMajorVersionRepairEnabled() &&
+ Gossiper.instance.hasMultipleLiveMajorVersions()) {
+ logger.info("Auto-repair is disabled when nodes in the cluster have different major versions");
Review Comment:
https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/metrics/AutoRepairMetrics.java
##########
test/unit/org/apache/cassandra/repair/autorepair/AutoRepairUtilsTest.java:
##########
@@ -232,6 +236,52 @@ public void testGetHostIdsInCurrentRing_multiple_nodes()
assertTrue(hosts.contains(hostId));
}
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithSingleNode()
+ {
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameVersion()
+ {
+ ClusterMetadataTestHelper.addEndpoint(2);
+ // Test the current behavior with the existing cluster setup
+ // In a single-node test environment, this should return false
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameMajorVersionDifferentMinorVersions()
+ {
+ // add two nodes with the current cassandra major version, but different minor version
+ CassandraVersion differentCassandraVersion = new CassandraVersion(
+ String.format("%d.%d",
+ NodeVersion.CURRENT.cassandraVersion.major,
+ NodeVersion.CURRENT.cassandraVersion.minor+1));
+ ClusterMetadataTestHelper.addEndpoint(2, new NodeVersion(
+ differentCassandraVersion,
+ NodeVersion.CURRENT_METADATA_VERSION));
+ // With the same major versions, but different minor versions, we should still see this function return true
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfDifferentMajorVersions()
+ {
+ // add two nodes with different cassandra major versions
+ CassandraVersion differentCassandraVersion = new CassandraVersion(
+ String.format("%d.%d", NodeVersion.CURRENT.cassandraVersion.major - 1, 0));
+ ClusterMetadataTestHelper.addEndpoint(2, new NodeVersion(differentCassandraVersion,
+ NodeVersion.CURRENT_METADATA_VERSION));
+ // With the same major versions, but different minor versions, we should still see this function return true
Review Comment:
nit typo: With the same minor versions, but different major versions
##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepair.java:
##########
@@ -165,6 +166,11 @@ public void repair(AutoRepairConfig.RepairType repairType)
logger.debug("Auto-repair is disabled for repair type {}", repairType);
return;
}
+ if (!DatabaseDescriptor.getMixedMajorVersionRepairEnabled() &&
+ Gossiper.instance.hasMultipleLiveMajorVersions()) {
+ logger.info("Auto-repair is disabled when nodes in the cluster have different major versions");
Review Comment:
The metric is still missing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2301741825
##########
test/unit/org/apache/cassandra/repair/autorepair/AutoRepairUtilsTest.java:
##########
@@ -232,6 +236,52 @@ public void testGetHostIdsInCurrentRing_multiple_nodes()
assertTrue(hosts.contains(hostId));
}
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithSingleNode()
+ {
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameVersion()
+ {
+ ClusterMetadataTestHelper.addEndpoint(2);
+ // Test the current behavior with the existing cluster setup
+ // In a single-node test environment, this should return false
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfSameMajorVersionDifferentMinorVersions()
+ {
+ // add two nodes with cassandra 5 major version, but different minor version
+ CassandraVersion differentCassandraVersion = new CassandraVersion(
+ String.format("%d.%d",
+ NodeVersion.CURRENT.cassandraVersion.major,
+ NodeVersion.CURRENT.cassandraVersion.minor+1));
+ ClusterMetadataTestHelper.addEndpoint(2, new NodeVersion(
+ differentCassandraVersion,
+ NodeVersion.CURRENT_METADATA_VERSION));
+ // With the same major versions, but different minor versions, we should still see this function return true
+ boolean result = AutoRepairUtils.hasMultipleLiveMajorVersions();
+ assertFalse(result);
+ }
+
+ @Test
+ public void testHasMultipleLiveMajorVersionsWithMultipleNodesOfDifferentMajorVersions()
+ {
+ // add two nodes with cassandra 5 major version, but different minor version
Review Comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "jaydeepkumar1984 (via GitHub)" <gi...@apache.org>.
jaydeepkumar1984 commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2271632169
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -374,6 +374,9 @@ public MemtableOptions()
// if you want to disable this feature (the recommendation is not to, but if you want to disable it for whatever reason) then set the ratio to 0.0
public volatile double incremental_repair_disk_headroom_reject_ratio = 0.2;
+ // by default repair is disabled if there are mixed major versions detected, but you can enable it using this flag
+ public volatile boolean mixed_major_version_repair_enabled = false;
Review Comment:
Can you add the configuration inside AutoRepairConfig.java?
##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepair.java:
##########
@@ -165,6 +166,11 @@ public void repair(AutoRepairConfig.RepairType repairType)
logger.debug("Auto-repair is disabled for repair type {}", repairType);
return;
}
+ if (!DatabaseDescriptor.getMixedMajorVersionRepairEnabled() &&
+ Gossiper.instance.hasMultipleLiveMajorVersions()) {
+ logger.info("Auto-repair is disabled when nodes in the cluster have different major versions");
Review Comment:
Please add a metric
##########
test/distributed/org/apache/cassandra/distributed/test/GossipTest.java:
##########
@@ -285,4 +288,40 @@ public void testQuarantine() throws IOException
assertTrue(removed);
}
}
+
+ @Test
+ public void testMultipleMajorVersionsWithMultipleMajorVersions() throws Exception
+ {
+ try (Cluster cluster = builder()
+ .withNodes(2)
+ .withConfig(config -> config.with(GOSSIP))
+ .start())
+ {
+ InetSocketAddress peer1 = cluster.get(1).broadcastAddress();
+ String version1 = "4.0.1920";
Review Comment:
Can you add a test case by setting different minor versions explicitly, say `4.0.1920`, `4.1.2`?
##########
src/java/org/apache/cassandra/gms/Gossiper.java:
##########
@@ -2034,6 +2034,23 @@ private boolean nodesAgreeOnSchema(Collection<InetAddressAndPort> nodes)
return true;
}
+ public boolean hasMultipleLiveMajorVersions()
Review Comment:
Can we extend the existing `AutoRepairUtils.java::getHostIdsInCurrentRing`, which already goes through all the nodes in a cluster?
https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/repair/autorepair/AutoRepairUtils.java#L429
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#issuecomment-3186371286
@jaydeepkumar1984 I have addressed all the feedback except metrics, which I can do in the next revision if you can point me to a code link.
For adding tests, I found this class called ClusterMetadataTestHelper, which I was able to use and extend. This worked really well. Let me know if there is any feedback with how I wrote those tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#issuecomment-3181244847
This is based off of the existing PR from clohfink@ - https://github.com/jaydeepkumar1984/cassandra/pull/20/commits/c63a5a4d9881417f89d5e9836dbb8045dcdb558e
Changes from that PR
* Used yaml config instead of guardrail
* Stop auto repair only, not the manual repair jobs
* Stop repair for multiple major versions only, and not minor versions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2304246107
##########
doc/modules/cassandra/pages/managing/operating/auto_repair.adoc:
##########
@@ -219,6 +219,10 @@ in nodetool repair. General advice is to keep this true.
| repair_retry_backoff | 30s | Backoff time before retrying a repair session.
| repair_task_min_duration | 5s | Minimum duration for the execution of a single repair task. This prevents the
scheduler from overwhelming the node by scheduling too many repair tasks in a short period of time.
+| mixed_major_version_repair_enabled | false | Enable/Disable running repairs on the cluster when there are mixed.
+major versions detected, which usually occurs when the cluster is being upgraded. Repair is not supported in this scenario
Review Comment:
```
Repair is not supported in this scenario as data might be streamed from a node with a different version of Cassandra leading to compatibility issues. Once major versions are compatible for streaming data, this setting can be turned to true.
```
Could we just rephrase this as:
```
Repairs between nodes of different major versions is not something that is tested, so this may lead to data compatibility issues. It is strongly discouraged to set this to true without doing extensive testing beforehand.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2304235069
##########
doc/modules/cassandra/pages/managing/operating/auto_repair.adoc:
##########
@@ -219,6 +219,10 @@ in nodetool repair. General advice is to keep this true.
| repair_retry_backoff | 30s | Backoff time before retrying a repair session.
| repair_task_min_duration | 5s | Minimum duration for the execution of a single repair task. This prevents the
scheduler from overwhelming the node by scheduling too many repair tasks in a short period of time.
+| mixed_major_version_repair_enabled | false | Enable/Disable running repairs on the cluster when there are mixed.
Review Comment:
Should remove extraneous `.` here.
```suggestion
| mixed_major_version_repair_enabled | false | Enable/Disable running repairs on the cluster when there are mixed
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2305581807
##########
src/java/org/apache/cassandra/tools/nodetool/SetAutoRepairConfig.java:
##########
@@ -176,6 +176,9 @@ public void execute(NodeProbe probe)
case "repair_retry_backoff":
probe.setAutoRepairRetryBackoff(repairTypeStr, paramVal);
break;
+ case "mixed_major_version_repair_enabled":
Review Comment:
Ah, also like the documentation, since this is not repairType driven, it should be moved to the switch statement starting at line 86 so we aren't required to provide a repairType to set this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#issuecomment-3229387823
Here's the test run using the latest commit - https://pre-ci.cassandra.apache.org/job/cassandra/lastBuild/testReport/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2275015250
##########
src/java/org/apache/cassandra/gms/Gossiper.java:
##########
@@ -2034,6 +2034,23 @@ private boolean nodesAgreeOnSchema(Collection<InetAddressAndPort> nodes)
return true;
}
+ public boolean hasMultipleLiveMajorVersions()
Review Comment:
Yes, added in new revision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "jaydeepkumar1984 (via GitHub)" <gi...@apache.org>.
jaydeepkumar1984 commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2320372253
##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepair.java:
##########
@@ -165,6 +166,11 @@ public void repair(AutoRepairConfig.RepairType repairType)
logger.debug("Auto-repair is disabled for repair type {}", repairType);
return;
}
+ if (!DatabaseDescriptor.getMixedMajorVersionRepairEnabled() &&
+ Gossiper.instance.hasMultipleLiveMajorVersions()) {
+ logger.info("Auto-repair is disabled when nodes in the cluster have different major versions");
Review Comment:
Sounds good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2299181481
##########
conf/cassandra.yaml:
##########
@@ -2779,6 +2779,9 @@ storage_compatibility_mode: NONE
# # for a specified duration to ensure they are indeed removed before adjustments are made to the schedule.
# history_clear_delete_hosts_buffer_interval: 2h
# # NOTE: Each of the below settings can be overridden per repair type under repair_type_overrides
+# # by default repair is disabled if there are mixed major versions detected - which would happen
+# # if a major version upgrade is being performed on the cluster, but a user can enable it using this flag
+# # mixed_major_version_repair_enabled = false;
Review Comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2307562174
##########
doc/modules/cassandra/pages/managing/operating/auto_repair.adoc:
##########
@@ -219,6 +219,10 @@ in nodetool repair. General advice is to keep this true.
| repair_retry_backoff | 30s | Backoff time before retrying a repair session.
| repair_task_min_duration | 5s | Minimum duration for the execution of a single repair task. This prevents the
scheduler from overwhelming the node by scheduling too many repair tasks in a short period of time.
+| mixed_major_version_repair_enabled | false | Enable/Disable running repairs on the cluster when there are mixed
+major versions detected, which usually occurs when the cluster is being upgraded. Repairs between nodes of
+different major versions is not something that is tested, so this may lead to data compatibility issues.
+It is strongly discouraged to set this to true without doing extensive testing beforehand.
Review Comment:
Good catch. These should be top level settings and not auto repair settings. Fixed.
##########
src/java/org/apache/cassandra/tools/nodetool/SetAutoRepairConfig.java:
##########
@@ -176,6 +176,9 @@ public void execute(NodeProbe probe)
case "repair_retry_backoff":
probe.setAutoRepairRetryBackoff(repairTypeStr, paramVal);
break;
+ case "mixed_major_version_repair_enabled":
Review Comment:
Good catch. These should be top level settings and not auto repair settings. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2305426895
##########
src/java/org/apache/cassandra/tools/nodetool/SetAutoRepairConfig.java:
##########
@@ -53,7 +53,7 @@ public class SetAutoRepairConfig extends AbstractCommand
"|allow_parallel_replica_repair|allow_parallel_repair_across_schedules" +
"|materialized_view_repair_enabled|repair_max_retries" +
"|repair_retry_backoff|repair_session_timeout|min_repair_task_duration" +
- "|repair_by_keyspace|token_range_splitter.<property>]" })
+ "|repair_by_keyspace|token_range_splitter.<property>]|mixed_major_version_repair_enabled" })
Review Comment:
```suggestion
"|repair_by_keyspace|mixed_major_version_repair_enabled|token_range_splitter.<property>]" })
```
Suggest moving it before `token_range_splitter.<property>` since that's a bit of an exceptional thing. Really we should order this alphabetically, but I think its fine as is for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2308120794
##########
src/java/org/apache/cassandra/service/AutoRepairService.java:
##########
@@ -102,6 +102,7 @@ public String getAutoRepairConfiguration()
appendConfig(sb, "repair_check_interval", config.getRepairCheckInterval());
appendConfig(sb, "repair_task_min_duration", config.getRepairTaskMinDuration());
appendConfig(sb, "history_clear_delete_hosts_buffer_interval", config.getAutoRepairHistoryClearDeleteHostsBufferInterval());
+ appendConfig(sb, "mixed", config.getMixedMajorVersionRepairEnabled());
Review Comment:
I somehow missed that 😭. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2274492668
##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepair.java:
##########
@@ -165,6 +166,11 @@ public void repair(AutoRepairConfig.RepairType repairType)
logger.debug("Auto-repair is disabled for repair type {}", repairType);
return;
}
+ if (!DatabaseDescriptor.getMixedMajorVersionRepairEnabled() &&
+ Gossiper.instance.hasMultipleLiveMajorVersions()) {
+ logger.info("Auto-repair is disabled when nodes in the cluster have different major versions");
Review Comment:
I am not seeing this class emit metrics. I'll be happy to introduce metrics to this class. Do you have an example of code where we do emit metric, so I can use that pattern to emit metrics?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2274488348
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -374,6 +374,9 @@ public MemtableOptions()
// if you want to disable this feature (the recommendation is not to, but if you want to disable it for whatever reason) then set the ratio to 0.0
public volatile double incremental_repair_disk_headroom_reject_ratio = 0.2;
+ // by default repair is disabled if there are mixed major versions detected, but you can enable it using this flag
+ public volatile boolean mixed_major_version_repair_enabled = false;
Review Comment:
Yes. Will address in next revision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#issuecomment-3226130821
@tolbertam Thanks for your feedback. I have addressed all of it except adding metrics. Let me know if that is a blocker and I can get to it next week.
If metrics is a blocker, can you share the metric name and purpose? That way, it is easier for me to add the metric.
Also, I have run pre-ci testing on the commit and the tests look good - https://pre-ci.cassandra.apache.org/job/cassandra/74/testReport/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2301735039
##########
test/distributed/org/apache/cassandra/distributed/test/log/ClusterMetadataTestHelper.java:
##########
@@ -798,19 +798,27 @@ public MoveProcess finishMove()
public static void addEndpoint(int i)
{
+ addEndpoint(i, NodeVersion.CURRENT);
+ }
+
+ public static void addEndpoint(int i, NodeVersion nodeVersion) {
try
Review Comment:
Fixed.
##########
test/distributed/org/apache/cassandra/distributed/test/log/ClusterMetadataTestHelper.java:
##########
@@ -798,19 +798,27 @@ public MoveProcess finishMove()
public static void addEndpoint(int i)
{
+ addEndpoint(i, NodeVersion.CURRENT);
+ }
+
+ public static void addEndpoint(int i, NodeVersion nodeVersion) {
try
{
- addEndpoint(InetAddressAndPort.getByName("127.0.0." + i), new Murmur3Partitioner.LongToken(i));
+ addEndpoint(InetAddressAndPort.getByName("127.0.0." + i), new Murmur3Partitioner.LongToken(i), nodeVersion);
}
catch (UnknownHostException e)
{
throw new RuntimeException(e);
}
}
- public static void addEndpoint(InetAddressAndPort endpoint, Token t)
+ public static void addEndpoint(InetAddressAndPort endpoint, Token t) {
+ addEndpoint(endpoint, t, NodeVersion.CURRENT);
Review Comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2301743834
##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepairUtils.java:
##########
@@ -425,6 +425,21 @@ public static CurrentRepairStatus getCurrentRepairStatus(RepairType repairType,
return null;
}
+ /**
+ * Checks whether the cluster has multiple major versions
+ * @return
+ * true if more than one major versions are detected
+ * false if only one major version is detected
+ *
+ */
+ public static boolean hasMultipleLiveMajorVersions()
+ {
+ ClusterMetadata metadata = ClusterMetadata.current();
+ int maxMajorVersion = ClusterMetadata.current().directory.clusterMaxVersion.cassandraVersion.major;
+ int minMajorVersion = ClusterMetadata.current().directory.clusterMinVersion.cassandraVersion.major;
+ return maxMajorVersion != minMajorVersion;
+ }
+
Review Comment:
Yes, I think so too. My understanding is that we only want to block repair on multiple major versions right now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#issuecomment-3233817398
Looks good, just tried it out and setautorepairconfig works without a repair type now. Added one note about tweaking a test, after that I think only thing that's missing now is that it's not present in `nodetool getautorepairconfig` output, mind adding that? I think just need to add something to `AutoRepairService.getAutoRepairConfiguration()` and that should do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Stop repair scheduler if two major versions are detected [cassandra]
Posted by "himanshujindal (via GitHub)" <gi...@apache.org>.
himanshujindal commented on code in PR #4304:
URL: https://github.com/apache/cassandra/pull/4304#discussion_r2314240069
##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepair.java:
##########
@@ -165,6 +166,11 @@ public void repair(AutoRepairConfig.RepairType repairType)
logger.debug("Auto-repair is disabled for repair type {}", repairType);
return;
}
+ if (!DatabaseDescriptor.getMixedMajorVersionRepairEnabled() &&
+ Gossiper.instance.hasMultipleLiveMajorVersions()) {
+ logger.info("Auto-repair is disabled when nodes in the cluster have different major versions");
Review Comment:
@jaydeepkumar1984 See my comment here - https://github.com/apache/cassandra/pull/4304#issuecomment-3226130821 and Andy's reply in slack - https://the-asf.slack.com/archives/C06MR8K2Q0Z/p1756306978752409?thread_ts=1755038748.234549&cid=C06MR8K2Q0Z
Happy to add metric if we align with my proposal there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org