From 32bec7bc25978c2178ed67de9c6eb775d680eb25 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 24 Mar 2026 16:25:05 -0700 Subject: [PATCH 01/27] added check to make sure ranges that are still used by Accord are not deleted by nodetool cleanup --- .../db/compaction/CompactionManager.java | 29 +++++ .../test/accord/AccordCQLTestBase.java | 7 + .../accord/AccordNodetoolCleanupTest.java | 123 ++++++++++++++++++ 3 files changed, 159 insertions(+) create mode 100644 test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index ea370b4601fb..6d67f1a98fad 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -65,6 +65,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import accord.primitives.Ranges; + import org.apache.cassandra.cache.AutoSavingCache; import org.apache.cassandra.concurrent.ExecutorFactory; import org.apache.cassandra.concurrent.WrappedExecutorPlus; @@ -114,9 +116,12 @@ import org.apache.cassandra.repair.NoSuchRepairSessionException; import org.apache.cassandra.schema.CompactionParams.TombstoneOption; import org.apache.cassandra.schema.Schema; +import org.apache.cassandra.schema.TableId; import org.apache.cassandra.schema.TableMetadata; import org.apache.cassandra.service.ActiveRepairService; import org.apache.cassandra.service.StorageService; +import org.apache.cassandra.service.accord.AccordService; +import org.apache.cassandra.service.accord.TokenRange; import org.apache.cassandra.streaming.PreviewKind; import org.apache.cassandra.tcm.ClusterMetadata; import org.apache.cassandra.tcm.ownership.DataPlacement; @@ -807,6 +812,21 @@ public Iterable filterSSTables(LifecycleTransaction transaction) SSTableReader sstable = sstableIter.next(); boolean needsCleanupFull = needsCleanup(sstable, fullRanges); boolean needsCleanupTransient = !transientRanges.isEmpty() && sstable.isRepaired() && needsCleanup(sstable, transientRanges); + boolean sstableRangesIntersectWithCommandStores = sstableContainsRangeNeededByAccord(cfStore.getTableId(), sstable); + + // If there still exists Command Stores that own this range, don't cleanup this specific range + // as Accord still needs it even though we no longer own the range + if (sstableRangesIntersectWithCommandStores) + { + logger.debug("Skipping {} ([{}, {}]) for cleanup; as Accord still needs the ranges.", + sstable, + sstable.getFirst().getToken(), + sstable.getLast().getToken()); + sstableIter.remove(); + transaction.cancel(sstable); + skippedSStables++; + } + //If there are no ranges for which the table needs cleanup either due to lack of intersection or lack //of the table being repaired. totalSSTables++; @@ -1599,6 +1619,15 @@ public static boolean needsCleanup(SSTableReader sstable, Collection builder + .withoutVNodes() + .withConfig(config -> + config + .set("accord.shard_durability_target_splits", "1") + .set("accord.shard_durability_cycle", "20s") + .with(Feature.NETWORK, Feature.GOSSIP)), 6); + } + + @Test + public void accordNodetoolCleanupTest() throws Throwable + { + List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', + "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 3}", + "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); + test(ddls, cluster -> { + cluster.coordinator(2).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); + cluster.coordinator(2).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 2, 2); + + SimpleQueryResult result = cluster.coordinator(2).executeWithResult("SELECT token(k) FROM " + qualifiedAccordTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + String keyspace = KEYSPACE; + String tableName = accordTableName; + cluster.get(2).flush(withKeyspace("%s")); + + assertEquals(1, (int) cluster.get(2).callOnInstance(() -> Keyspace.open(keyspace).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + String originalToken = cluster.get(2).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); + + long token = (Long) result.toObjectArrays()[0][0]; + + assert(token < Long.parseLong(originalToken)); + + cluster.get(2).runOnInstance(() -> { + StorageService.instance.move(Long.toString(token - 10000)); + }); + + NodeToolResult r = cluster.get(2).nodetoolResult("cleanup", KEYSPACE, accordTableName); + + assertEquals(1, (int) cluster.get(2).callOnInstance(() -> Keyspace.open(keyspace).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + // The invariant that we want to preserve is that no ranges that overlap with our command stores should be removed, in this case this so happens to be + cluster.get(2).runOnInstance(() -> { + Ranges commandStoreRanges = Ranges.EMPTY; + for (CommandStore commandStore : AccordService.instance().node().commandStores().all()) + { + Ranges commandStoreRange = getBlocking(commandStore.submit((PreLoadContext.Empty) () -> "Get ranges", safeCommandStore -> { + return safeCommandStore.ranges().all(); + })); + + commandStoreRanges = commandStoreRanges.union(AbstractRanges.UnionMode.MERGE_ADJACENT, commandStoreRange); + } + }); + }); + } +} From b072804482d2c7d02c009496add689a95ac90c70 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 24 Mar 2026 16:33:39 -0700 Subject: [PATCH 02/27] special case sstable with single key --- .../cassandra/db/compaction/CompactionManager.java | 13 ++++++++----- .../test/accord/AccordNodetoolCleanupTest.java | 8 -------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index 6d67f1a98fad..2f2e6dd3dde4 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -122,6 +122,7 @@ import org.apache.cassandra.service.StorageService; import org.apache.cassandra.service.accord.AccordService; import org.apache.cassandra.service.accord.TokenRange; +import org.apache.cassandra.service.accord.api.TokenKey; import org.apache.cassandra.streaming.PreviewKind; import org.apache.cassandra.tcm.ClusterMetadata; import org.apache.cassandra.tcm.ownership.DataPlacement; @@ -812,7 +813,7 @@ public Iterable filterSSTables(LifecycleTransaction transaction) SSTableReader sstable = sstableIter.next(); boolean needsCleanupFull = needsCleanup(sstable, fullRanges); boolean needsCleanupTransient = !transientRanges.isEmpty() && sstable.isRepaired() && needsCleanup(sstable, transientRanges); - boolean sstableRangesIntersectWithCommandStores = sstableContainsRangeNeededByAccord(cfStore.getTableId(), sstable); + boolean sstableRangesIntersectWithCommandStores = sstableContainsRangesNeededByAccord(cfStore.getTableId(), sstable); // If there still exists Command Stores that own this range, don't cleanup this specific range // as Accord still needs it even though we no longer own the range @@ -1619,13 +1620,15 @@ public static boolean needsCleanup(SSTableReader sstable, Collection { cluster.coordinator(2).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); - cluster.coordinator(2).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 2, 2); SimpleQueryResult result = cluster.coordinator(2).executeWithResult("SELECT token(k) FROM " + qualifiedAccordTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); From e3bd0f873ef55154ee55b337f785a6709ab9641f Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 24 Mar 2026 16:35:12 -0700 Subject: [PATCH 03/27] removed unused imports --- .../distributed/test/accord/AccordCQLTestBase.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java index 0ed8cb5facd6..dbf6d6553f64 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java @@ -39,7 +39,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import static com.google.common.collect.Iterables.getOnlyElement; import org.assertj.core.api.Assertions; import org.junit.BeforeClass; @@ -48,9 +47,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import accord.local.CommandStore; -import accord.local.PreLoadContext; -import accord.primitives.Ranges; import accord.primitives.Unseekables; import accord.topology.SelectShards; import accord.topology.Topologies; @@ -77,14 +73,12 @@ import org.apache.cassandra.distributed.Cluster; import org.apache.cassandra.distributed.api.ConsistencyLevel; import org.apache.cassandra.distributed.api.ICoordinator; -import org.apache.cassandra.distributed.api.NodeToolResult; import org.apache.cassandra.distributed.api.QueryResults; import org.apache.cassandra.distributed.api.SimpleQueryResult; import org.apache.cassandra.distributed.shared.AssertUtils; import org.apache.cassandra.distributed.test.sai.SAIUtil; import org.apache.cassandra.distributed.util.QueryResultUtil; import org.apache.cassandra.exceptions.InvalidRequestException; -import org.apache.cassandra.service.StorageService; import org.apache.cassandra.service.accord.AccordService; import org.apache.cassandra.service.accord.AccordTestUtils; import org.apache.cassandra.service.consensus.TransactionalMode; @@ -105,7 +99,6 @@ import static org.apache.cassandra.distributed.api.Feature.NETWORK; import static org.apache.cassandra.distributed.shared.AssertUtils.assertRows; import static org.apache.cassandra.distributed.util.QueryResultUtil.assertThat; -import static org.apache.cassandra.service.accord.AccordService.getBlocking; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; From 3d752e3eca6ab7e4649a1c3d48e6eb08ac08f609 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 24 Mar 2026 16:42:18 -0700 Subject: [PATCH 04/27] fix up tests --- .../accord/AccordNodetoolCleanupTest.java | 38 ++++++++++++------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index cd97d090fe54..251f083407ef 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -22,18 +22,23 @@ import java.util.Arrays; import java.util.List; +import accord.api.RoutingKey; import accord.local.CommandStore; import accord.local.PreLoadContext; import accord.primitives.AbstractRanges; import accord.primitives.Ranges; import org.apache.cassandra.db.Keyspace; +import org.apache.cassandra.dht.Murmur3Partitioner; import org.apache.cassandra.distributed.api.ConsistencyLevel; import org.apache.cassandra.distributed.api.Feature; import org.apache.cassandra.distributed.api.NodeToolResult; import org.apache.cassandra.distributed.api.SimpleQueryResult; +import org.apache.cassandra.schema.Schema; +import org.apache.cassandra.schema.TableId; import org.apache.cassandra.service.StorageService; import org.apache.cassandra.service.accord.AccordService; +import org.apache.cassandra.service.accord.api.TokenKey; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -41,6 +46,7 @@ import static org.apache.cassandra.service.accord.AccordService.getBlocking; import static com.google.common.collect.Iterables.getOnlyElement; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import org.junit.BeforeClass; import org.junit.Test; @@ -74,15 +80,15 @@ public void accordNodetoolCleanupTest() throws Throwable "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 3}", "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); test(ddls, cluster -> { + String tableName = accordTableName; + cluster.coordinator(2).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); SimpleQueryResult result = cluster.coordinator(2).executeWithResult("SELECT token(k) FROM " + qualifiedAccordTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); - String keyspace = KEYSPACE; - String tableName = accordTableName; cluster.get(2).flush(withKeyspace("%s")); - assertEquals(1, (int) cluster.get(2).callOnInstance(() -> Keyspace.open(keyspace).getColumnFamilyStore(tableName).getLiveSSTables().size())); + assertEquals(1, (int) cluster.get(2).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); String originalToken = cluster.get(2).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); @@ -91,25 +97,31 @@ public void accordNodetoolCleanupTest() throws Throwable assert(token < Long.parseLong(originalToken)); cluster.get(2).runOnInstance(() -> { - StorageService.instance.move(Long.toString(token - 10000)); - }); + TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); + RoutingKey key = TokenKey.parse(tid, String.valueOf(token), Murmur3Partitioner.instance); - NodeToolResult r = cluster.get(2).nodetoolResult("cleanup", KEYSPACE, accordTableName); - - assertEquals(1, (int) cluster.get(2).callOnInstance(() -> Keyspace.open(keyspace).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - // The invariant that we want to preserve is that no ranges that overlap with our command stores should be removed, in this case this so happens to be - cluster.get(2).runOnInstance(() -> { - Ranges commandStoreRanges = Ranges.EMPTY; + boolean tokenInCommandStore = false; for (CommandStore commandStore : AccordService.instance().node().commandStores().all()) { Ranges commandStoreRange = getBlocking(commandStore.submit((PreLoadContext.Empty) () -> "Get ranges", safeCommandStore -> { return safeCommandStore.ranges().all(); })); - commandStoreRanges = commandStoreRanges.union(AbstractRanges.UnionMode.MERGE_ADJACENT, commandStoreRange); + if (commandStoreRange.intersects(key)) + tokenInCommandStore = true; } + + assertTrue(tokenInCommandStore); + }); + + cluster.get(2).runOnInstance(() -> { + StorageService.instance.move(Long.toString(token - 1000)); }); + + cluster.get(2).nodetool("cleanup", KEYSPACE, accordTableName); + + assertEquals(1, (int) cluster.get(2).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + }); } } From b20db8fd819e25c437ee1ca99dfd14507472d271 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 24 Mar 2026 16:53:57 -0700 Subject: [PATCH 05/27] comments --- .../cassandra/db/compaction/CompactionManager.java | 2 +- .../test/accord/AccordNodetoolCleanupTest.java | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index 2f2e6dd3dde4..18c28ad0490e 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -1622,7 +1622,7 @@ public static boolean needsCleanup(SSTableReader sstable, Collection Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - String originalToken = cluster.get(2).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); long token = (Long) result.toObjectArrays()[0][0]; assert(token < Long.parseLong(originalToken)); + assertEquals(1, (int) cluster.get(2).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + // SSTable overlaps with Accord CommandStores ranges, so after cleanup we should still have that + // 1 SSTable, even though we no longer own the ranges for that SSTable cluster.get(2).runOnInstance(() -> { TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); RoutingKey key = TokenKey.parse(tid, String.valueOf(token), Murmur3Partitioner.instance); @@ -103,7 +103,7 @@ public void accordNodetoolCleanupTest() throws Throwable boolean tokenInCommandStore = false; for (CommandStore commandStore : AccordService.instance().node().commandStores().all()) { - Ranges commandStoreRange = getBlocking(commandStore.submit((PreLoadContext.Empty) () -> "Get ranges", safeCommandStore -> { + Ranges commandStoreRange = getBlocking(commandStore.submit((PreLoadContext.Empty) () -> "Get CommandStore ranges", safeCommandStore -> { return safeCommandStore.ranges().all(); })); @@ -121,7 +121,6 @@ public void accordNodetoolCleanupTest() throws Throwable cluster.get(2).nodetool("cleanup", KEYSPACE, accordTableName); assertEquals(1, (int) cluster.get(2).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - }); } } From 5a892a48a68dcdf856118ed0f157a5c6914e4957 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Wed, 25 Mar 2026 13:38:55 -0700 Subject: [PATCH 06/27] moved test to cql test base --- .../test/accord/AccordCQLTestBase.java | 64 +++++++++ .../accord/AccordNodetoolCleanupTest.java | 126 ------------------ 2 files changed, 64 insertions(+), 126 deletions(-) delete mode 100644 test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java index dbf6d6553f64..fcaae0fc1f81 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java @@ -39,6 +39,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import static com.google.common.collect.Iterables.getOnlyElement; import org.assertj.core.api.Assertions; import org.junit.BeforeClass; @@ -47,6 +48,10 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import accord.api.RoutingKey; +import accord.local.CommandStore; +import accord.local.PreLoadContext; +import accord.primitives.Ranges; import accord.primitives.Unseekables; import accord.topology.SelectShards; import accord.topology.Topologies; @@ -64,6 +69,7 @@ import org.apache.cassandra.cql3.ast.Txn; import org.apache.cassandra.cql3.functions.types.utils.Bytes; import org.apache.cassandra.cql3.statements.TransactionStatement; +import org.apache.cassandra.db.Keyspace; import org.apache.cassandra.db.marshal.Int32Type; import org.apache.cassandra.db.marshal.ListType; import org.apache.cassandra.db.marshal.MapType; @@ -79,8 +85,12 @@ import org.apache.cassandra.distributed.test.sai.SAIUtil; import org.apache.cassandra.distributed.util.QueryResultUtil; import org.apache.cassandra.exceptions.InvalidRequestException; +import org.apache.cassandra.schema.Schema; +import org.apache.cassandra.schema.TableId; +import org.apache.cassandra.service.StorageService; import org.apache.cassandra.service.accord.AccordService; import org.apache.cassandra.service.accord.AccordTestUtils; +import org.apache.cassandra.service.accord.api.TokenKey; import org.apache.cassandra.service.consensus.TransactionalMode; import org.apache.cassandra.service.consensus.migration.TransactionalMigrationFromMode; import org.apache.cassandra.utils.AssertionUtils; @@ -99,6 +109,7 @@ import static org.apache.cassandra.distributed.api.Feature.NETWORK; import static org.apache.cassandra.distributed.shared.AssertUtils.assertRows; import static org.apache.cassandra.distributed.util.QueryResultUtil.assertThat; +import static org.apache.cassandra.service.accord.AccordService.getBlocking; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -3433,4 +3444,57 @@ public void userSeesInvalidRejection() throws Exception .hasMessage("Attempted to set an element on a list which is null"); }); } + + @Test + public void accordNodetoolCleanupTest() throws Throwable + { + List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', + "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}", + "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH " + transactionalMode.asCqlParam()); + test(ddls, cluster -> { + String tableName = accordTableName; + + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); + + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedAccordTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + cluster.get(1).flush(withKeyspace("%s")); + + String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); + + long token = (Long) result.toObjectArrays()[0][0]; + + assert(token < Long.parseLong(originalToken)); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + // SSTable overlaps with Accord CommandStores ranges, so after cleanup we should still have that + // 1 SSTable, even though we no longer own the ranges for that SSTable + cluster.get(1).runOnInstance(() -> { + TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); + RoutingKey key = TokenKey.parse(tid, String.valueOf(token), Murmur3Partitioner.instance); + + boolean tokenInCommandStore = false; + for (CommandStore commandStore : AccordService.instance().node().commandStores().all()) + { + Ranges commandStoreRange = getBlocking(commandStore.submit((PreLoadContext.Empty) () -> "Get CommandStore ranges", safeCommandStore -> { + return safeCommandStore.ranges().all(); + })); + + if (commandStoreRange.intersects(key)) + tokenInCommandStore = true; + } + + assertTrue(tokenInCommandStore); + }); + + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(Long.toString(token - 1000)); + }); + + cluster.get(1).nodetool("cleanup", KEYSPACE, accordTableName); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + }); + } } diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java deleted file mode 100644 index 9fa0f9573a0b..000000000000 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ /dev/null @@ -1,126 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.cassandra.distributed.test.accord; - -import java.io.IOException; -import java.util.Arrays; -import java.util.List; - -import accord.api.RoutingKey; -import accord.local.CommandStore; -import accord.local.PreLoadContext; -import accord.primitives.Ranges; - -import org.apache.cassandra.db.Keyspace; -import org.apache.cassandra.dht.Murmur3Partitioner; -import org.apache.cassandra.distributed.api.ConsistencyLevel; -import org.apache.cassandra.distributed.api.Feature; -import org.apache.cassandra.distributed.api.SimpleQueryResult; -import org.apache.cassandra.schema.Schema; -import org.apache.cassandra.schema.TableId; -import org.apache.cassandra.service.StorageService; -import org.apache.cassandra.service.accord.AccordService; -import org.apache.cassandra.service.accord.api.TokenKey; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import static org.apache.cassandra.service.accord.AccordService.getBlocking; -import static com.google.common.collect.Iterables.getOnlyElement; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; - -import org.junit.BeforeClass; -import org.junit.Test; - -public class AccordNodetoolCleanupTest extends AccordTestBase -{ - private static final Logger logger = LoggerFactory.getLogger(AccordNodetoolCleanupTest.class); - - @Override - protected Logger logger() - { - return logger; - } - - @BeforeClass - public static void setupClass() throws IOException - { - AccordTestBase.setupCluster(builder -> builder - .withoutVNodes() - .withConfig(config -> - config - .set("accord.shard_durability_target_splits", "1") - .set("accord.shard_durability_cycle", "20s") - .with(Feature.NETWORK, Feature.GOSSIP)), 6); - } - - @Test - public void accordNodetoolCleanupTest() throws Throwable - { - List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', - "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 3}", - "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); - test(ddls, cluster -> { - String tableName = accordTableName; - - cluster.coordinator(2).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); - - SimpleQueryResult result = cluster.coordinator(2).executeWithResult("SELECT token(k) FROM " + qualifiedAccordTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); - - cluster.get(2).flush(withKeyspace("%s")); - - String originalToken = cluster.get(2).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); - - long token = (Long) result.toObjectArrays()[0][0]; - - assert(token < Long.parseLong(originalToken)); - - assertEquals(1, (int) cluster.get(2).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - // SSTable overlaps with Accord CommandStores ranges, so after cleanup we should still have that - // 1 SSTable, even though we no longer own the ranges for that SSTable - cluster.get(2).runOnInstance(() -> { - TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); - RoutingKey key = TokenKey.parse(tid, String.valueOf(token), Murmur3Partitioner.instance); - - boolean tokenInCommandStore = false; - for (CommandStore commandStore : AccordService.instance().node().commandStores().all()) - { - Ranges commandStoreRange = getBlocking(commandStore.submit((PreLoadContext.Empty) () -> "Get CommandStore ranges", safeCommandStore -> { - return safeCommandStore.ranges().all(); - })); - - if (commandStoreRange.intersects(key)) - tokenInCommandStore = true; - } - - assertTrue(tokenInCommandStore); - }); - - cluster.get(2).runOnInstance(() -> { - StorageService.instance.move(Long.toString(token - 1000)); - }); - - cluster.get(2).nodetool("cleanup", KEYSPACE, accordTableName); - - assertEquals(1, (int) cluster.get(2).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - }); - } -} From ecbf66f1257ecb874ea527822a30b02f6dce34e7 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Wed, 25 Mar 2026 13:55:05 -0700 Subject: [PATCH 07/27] add additional test and check if accord service is on --- .../db/compaction/CompactionManager.java | 3 ++ .../test/accord/AccordCQLTestBase.java | 36 ++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index 18c28ad0490e..de46469a1d37 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -1622,6 +1622,9 @@ public static boolean needsCleanup(SSTableReader sstable, Collection Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); @@ -3497,4 +3497,38 @@ public void accordNodetoolCleanupTest() throws Throwable assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); }); } + + @Test + public void nodetoolCleanupForNonAccordTableTest() throws Throwable + { + List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', + "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}", + "CREATE TABLE " + qualifiedRegularTableName + " (k int PRIMARY KEY, v int)"); + test(ddls, cluster -> { + String tableName = regularTableName; + + cluster.coordinator(1).execute("INSERT INTO " + qualifiedRegularTableName + " (k, v) VALUES (?, ?)", ConsistencyLevel.ALL, 1, 2); + + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedRegularTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + cluster.get(1).flush(withKeyspace("%s")); + + String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); + + long token = (Long) result.toObjectArrays()[0][0]; + + assertTrue(token < Long.parseLong(originalToken)); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(Long.toString(token - 1000)); + }); + + cluster.get(1).nodetool("cleanup", KEYSPACE, regularTableName); + + // SSTable should be removed as we no longer own the range and no Accord CommandStore needs these ranges + assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + }); + } } From 33ecb950babe8e0d849f93ac1a982653f83f53e9 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Wed, 1 Apr 2026 01:00:06 -0700 Subject: [PATCH 08/27] cleanup on retirement rather than on command store deletion --- .../db/compaction/CompactionManager.java | 95 ++++++++-- .../test/accord/AccordCQLTestBase.java | 98 ---------- .../accord/AccordNodetoolCleanupTest.java | 175 ++++++++++++++++++ 3 files changed, 253 insertions(+), 115 deletions(-) create mode 100644 test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index de46469a1d37..336f07533d49 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -65,6 +65,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import accord.primitives.AbstractRanges; import accord.primitives.Ranges; import org.apache.cassandra.cache.AutoSavingCache; @@ -618,7 +619,12 @@ public Object call() throws Exception FBUtilities.waitOnFutures(futures); assert compacting.originals().isEmpty(); logger.info("Finished {} for {}.{} successfully", operationType, keyspace, table); - return AllSSTableOpStatus.SUCCESSFUL; + + // Some SSTables were not fully cleaned up because Accord is still using those ranges + if (operation.incompleteOperation()) + return AllSSTableOpStatus.INCOMPLETE; + else + return AllSSTableOpStatus.SUCCESSFUL; } finally { @@ -640,15 +646,18 @@ public Object call() throws Exception private static interface OneSSTableOperation { + Iterable filterSSTables(LifecycleTransaction transaction); void execute(LifecycleTransaction input) throws IOException; + boolean incompleteOperation(); } public enum AllSSTableOpStatus { SUCCESSFUL(0), ABORTED(1), - UNABLE_TO_CANCEL(2); + UNABLE_TO_CANCEL(2), + INCOMPLETE(3); public final int statusCode; @@ -673,6 +682,12 @@ public void execute(LifecycleTransaction input) { scrubOne(cfs, input, options, active); } + + @Override + public boolean incompleteOperation() + { + return false; + } }, jobs, OperationType.SCRUB); } @@ -701,6 +716,12 @@ public void execute(LifecycleTransaction input) { verifyOne(cfs, input.onlyOne(), options, active); } + + @Override + public boolean incompleteOperation() + { + return false; + } }, 0, OperationType.VERIFY); } @@ -765,6 +786,12 @@ public void execute(LifecycleTransaction txn) task.setCompactionType(OperationType.UPGRADE_SSTABLES); task.execute(active); } + + @Override + public boolean incompleteOperation() + { + return false; + } }, jobs, OperationType.UPGRADE_SSTABLES); } @@ -801,6 +828,8 @@ public AllSSTableOpStatus performCleanup(final ColumnFamilyStore cfStore, int jo return parallelAllSSTableOperation(cfStore, new OneSSTableOperation() { + boolean incompleteOperation = false; + @Override public Iterable filterSSTables(LifecycleTransaction transaction) { @@ -808,16 +837,15 @@ public Iterable filterSSTables(LifecycleTransaction transaction) Iterator sstableIter = sortedSSTables.iterator(); int totalSSTables = 0; int skippedSStables = 0; + int sstablesInUseByAccord = 0; while (sstableIter.hasNext()) { SSTableReader sstable = sstableIter.next(); boolean needsCleanupFull = needsCleanup(sstable, fullRanges); boolean needsCleanupTransient = !transientRanges.isEmpty() && sstable.isRepaired() && needsCleanup(sstable, transientRanges); - boolean sstableRangesIntersectWithCommandStores = sstableContainsRangesNeededByAccord(cfStore.getTableId(), sstable); + totalSSTables++; - // If there still exists Command Stores that own this range, don't cleanup this specific range - // as Accord still needs it even though we no longer own the range - if (sstableRangesIntersectWithCommandStores) + if (sstableContainsRangesNeededByAccord(cfStore.getTableId(), sstable)) { logger.debug("Skipping {} ([{}, {}]) for cleanup; as Accord still needs the ranges.", sstable, @@ -825,13 +853,12 @@ public Iterable filterSSTables(LifecycleTransaction transaction) sstable.getLast().getToken()); sstableIter.remove(); transaction.cancel(sstable); - skippedSStables++; + sstablesInUseByAccord++; + incompleteOperation = true; } - //If there are no ranges for which the table needs cleanup either due to lack of intersection or lack //of the table being repaired. - totalSSTables++; - if (!needsCleanupFull && !needsCleanupTransient) + else if (!needsCleanupFull && !needsCleanupTransient) { logger.debug("Skipping {} ([{}, {}]) for cleanup; all rows should be kept. Needs cleanup full ranges: {} Needs cleanup transient ranges: {} Repaired: {}", sstable, @@ -845,8 +872,13 @@ public Iterable filterSSTables(LifecycleTransaction transaction) skippedSStables++; } } - logger.info("Skipping cleanup for {}/{} sstables for {}.{} since they are fully contained in owned ranges (full ranges: {}, transient ranges: {})", - skippedSStables, totalSSTables, cfStore.getKeyspaceName(), cfStore.getTableName(), fullRanges, transientRanges); + + logger.info("Skipping cleanup for {} sstables for {}.{} since they are fully contained in owned ranges (full ranges: {}, transient ranges: {})", + skippedSStables, cfStore.getKeyspaceName(), cfStore.getTableName(), fullRanges, transientRanges); + logger.info("Skipping cleanup for {} sstables for {}.{} since they are still being used by Accord", + sstablesInUseByAccord, cfStore.getKeyspaceName(), cfStore.getTableName()); + logger.info("Cleaned up {} sstables for {}.{}", totalSSTables, cfStore.getKeyspaceName(), cfStore.getTableName()); + sortedSSTables.sort(SSTableReader.sizeComparator); return sortedSSTables; } @@ -857,6 +889,12 @@ public void execute(LifecycleTransaction txn) throws IOException CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfStore, allRanges, transientRanges, txn.onlyOne().isRepaired(), FBUtilities.nowInSeconds()); doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes); } + + @Override + public boolean incompleteOperation() + { + return incompleteOperation; + } }, jobs, OperationType.CLEANUP); } @@ -924,6 +962,12 @@ protected int getLevel() task.setCompactionType(OperationType.GARBAGE_COLLECT); task.execute(active); } + + @Override + public boolean incompleteOperation() + { + return false; + } }, jobs, OperationType.GARBAGE_COLLECT); } @@ -992,6 +1036,12 @@ public void execute(LifecycleTransaction txn) task.setCompactionType(OperationType.RELOCATE); task.execute(active); } + + @Override + public boolean incompleteOperation() + { + return false; + } }, jobs, OperationType.RELOCATE); } @@ -1625,13 +1675,24 @@ public static boolean sstableContainsRangesNeededByAccord(TableId tableId, SSTab if (!AccordService.isSetup()) return false; - Ranges accordOwnedRanges = AccordService.instance().node().commandStores().commandStoresOwnedRanges(); + Future> futureAccordOwnedRanges = AccordService.toFuture(AccordService.instance().node().commandStores().getInUseRanges()); + logger.info("Waiting for Accord to be ready"); + + try + { + Ranges accordOwnedRanges = futureAccordOwnedRanges.get().stream().reduce(Ranges.EMPTY, (acc, r) -> acc.union(AbstractRanges.UnionMode.MERGE_ADJACENT, r)); - if (sstable.getFirst().equals(sstable.getLast())) - return accordOwnedRanges.intersects(new TokenKey(tableId, sstable.getFirst().getToken())); + if (sstable.getFirst().equals(sstable.getLast())) + return accordOwnedRanges.intersects(new TokenKey(tableId, sstable.getFirst().getToken())); - Ranges sstableRanges = Ranges.of(TokenRange.create(tableId, sstable.getFirst().getToken(), sstable.getLast().getToken())); - return accordOwnedRanges.intersects(sstableRanges); + Ranges sstableRanges = Ranges.of(TokenRange.create(tableId, sstable.getFirst().getToken(), sstable.getLast().getToken())); + return accordOwnedRanges.intersects(sstableRanges); + } + catch (Throwable e) + { + logger.error("Error while waiting for Accord in use ranges", e); + return true; + } } /** diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java index 40be8699eb04..dbf6d6553f64 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java @@ -39,7 +39,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import static com.google.common.collect.Iterables.getOnlyElement; import org.assertj.core.api.Assertions; import org.junit.BeforeClass; @@ -48,10 +47,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import accord.api.RoutingKey; -import accord.local.CommandStore; -import accord.local.PreLoadContext; -import accord.primitives.Ranges; import accord.primitives.Unseekables; import accord.topology.SelectShards; import accord.topology.Topologies; @@ -69,7 +64,6 @@ import org.apache.cassandra.cql3.ast.Txn; import org.apache.cassandra.cql3.functions.types.utils.Bytes; import org.apache.cassandra.cql3.statements.TransactionStatement; -import org.apache.cassandra.db.Keyspace; import org.apache.cassandra.db.marshal.Int32Type; import org.apache.cassandra.db.marshal.ListType; import org.apache.cassandra.db.marshal.MapType; @@ -85,12 +79,8 @@ import org.apache.cassandra.distributed.test.sai.SAIUtil; import org.apache.cassandra.distributed.util.QueryResultUtil; import org.apache.cassandra.exceptions.InvalidRequestException; -import org.apache.cassandra.schema.Schema; -import org.apache.cassandra.schema.TableId; -import org.apache.cassandra.service.StorageService; import org.apache.cassandra.service.accord.AccordService; import org.apache.cassandra.service.accord.AccordTestUtils; -import org.apache.cassandra.service.accord.api.TokenKey; import org.apache.cassandra.service.consensus.TransactionalMode; import org.apache.cassandra.service.consensus.migration.TransactionalMigrationFromMode; import org.apache.cassandra.utils.AssertionUtils; @@ -109,7 +99,6 @@ import static org.apache.cassandra.distributed.api.Feature.NETWORK; import static org.apache.cassandra.distributed.shared.AssertUtils.assertRows; import static org.apache.cassandra.distributed.util.QueryResultUtil.assertThat; -import static org.apache.cassandra.service.accord.AccordService.getBlocking; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -3444,91 +3433,4 @@ public void userSeesInvalidRejection() throws Exception .hasMessage("Attempted to set an element on a list which is null"); }); } - - @Test - public void accordNodetoolCleanupTest() throws Throwable - { - List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', - "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}", - "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH " + transactionalMode.asCqlParam()); - test(ddls, cluster -> { - String tableName = accordTableName; - - cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); - - SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedAccordTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); - - cluster.get(1).flush(withKeyspace("%s")); - - String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); - - long token = (Long) result.toObjectArrays()[0][0]; - - assertTrue(token < Long.parseLong(originalToken)); - - assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - // SSTable overlaps with Accord CommandStores ranges, so after cleanup we should still have that - // 1 SSTable, even though we no longer own the ranges for that SSTable - cluster.get(1).runOnInstance(() -> { - TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); - RoutingKey key = TokenKey.parse(tid, String.valueOf(token), Murmur3Partitioner.instance); - - boolean tokenInCommandStore = false; - for (CommandStore commandStore : AccordService.instance().node().commandStores().all()) - { - Ranges commandStoreRange = getBlocking(commandStore.submit((PreLoadContext.Empty) () -> "Get CommandStore ranges", safeCommandStore -> { - return safeCommandStore.ranges().all(); - })); - - if (commandStoreRange.intersects(key)) - tokenInCommandStore = true; - } - - assertTrue(tokenInCommandStore); - }); - - cluster.get(1).runOnInstance(() -> { - StorageService.instance.move(Long.toString(token - 1000)); - }); - - cluster.get(1).nodetool("cleanup", KEYSPACE, accordTableName); - - assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - }); - } - - @Test - public void nodetoolCleanupForNonAccordTableTest() throws Throwable - { - List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', - "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}", - "CREATE TABLE " + qualifiedRegularTableName + " (k int PRIMARY KEY, v int)"); - test(ddls, cluster -> { - String tableName = regularTableName; - - cluster.coordinator(1).execute("INSERT INTO " + qualifiedRegularTableName + " (k, v) VALUES (?, ?)", ConsistencyLevel.ALL, 1, 2); - - SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedRegularTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); - - cluster.get(1).flush(withKeyspace("%s")); - - String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); - - long token = (Long) result.toObjectArrays()[0][0]; - - assertTrue(token < Long.parseLong(originalToken)); - - assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - cluster.get(1).runOnInstance(() -> { - StorageService.instance.move(Long.toString(token - 1000)); - }); - - cluster.get(1).nodetool("cleanup", KEYSPACE, regularTableName); - - // SSTable should be removed as we no longer own the range and no Accord CommandStore needs these ranges - assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - }); - } } diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java new file mode 100644 index 000000000000..89b7effe4512 --- /dev/null +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.distributed.test.accord; + +import java.io.IOException; +import java.util.Arrays; +import java.util.List; + +import org.apache.cassandra.db.Keyspace; +import org.apache.cassandra.distributed.api.ConsistencyLevel; +import org.apache.cassandra.distributed.api.Feature; +import org.apache.cassandra.distributed.api.SimpleQueryResult; +import org.apache.cassandra.service.StorageService; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static com.google.common.collect.Iterables.getOnlyElement; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.BeforeClass; +import org.junit.Test; + +public class AccordNodetoolCleanupTest extends AccordTestBase +{ + private static final Logger logger = LoggerFactory.getLogger(AccordNodetoolCleanupTest.class); + + @Override + protected Logger logger() + { + return logger; + } + + @BeforeClass + public static void setupClass() throws IOException + { + AccordTestBase.setupCluster(builder -> builder + .withoutVNodes() + .withConfig(config -> + config + .set("accord.shard_durability_target_splits", "1") + .set("accord.shard_durability_cycle", "20s") + .with(Feature.NETWORK, Feature.GOSSIP)), 2); + } + + @Test + public void accordNodetoolCleanupTest() throws Throwable + { + List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', + "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}", + "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); + test(ddls, cluster -> { + + String tableName = accordTableName; + + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); + + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedAccordTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + cluster.get(1).flush(withKeyspace("%s")); + + String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); + + long token = (Long) result.toObjectArrays()[0][0]; + + assertTrue(token < Long.parseLong(originalToken)); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(Long.toString(token - 1000)); + }); + + // Wait until Accord retires range + try + { + Thread.sleep(20000); + } + catch (InterruptedException e) + { + fail(); + } + + cluster.get(1).nodetool("cleanup", KEYSPACE, accordTableName); + + assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + }); + } + + @Test + public void accordNodetoolCleanupRangeInUseTest() throws Throwable + { + List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', + "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}", + "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); + test(ddls, cluster -> { + + String tableName = accordTableName; + + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); + + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedAccordTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + cluster.get(1).flush(withKeyspace("%s")); + + String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); + + long token = (Long) result.toObjectArrays()[0][0]; + + assertTrue(token < Long.parseLong(originalToken)); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + cluster.get(1).runOnInstance(() -> StorageService.instance.move(Long.toString(token - 1000))); + + String accordTableName = qualifiedAccordTableName; + + cluster.get(1).nodetool("cleanup", KEYSPACE, accordTableName); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + }); + } + + @Test + public void nodetoolCleanupForNonAccordTableTest() throws Throwable + { + List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', + "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}", + "CREATE TABLE " + qualifiedRegularTableName + " (k int PRIMARY KEY, v int)"); + test(ddls, cluster -> { + String tableName = regularTableName; + + cluster.coordinator(1).execute("INSERT INTO " + qualifiedRegularTableName + " (k, v) VALUES (?, ?)", ConsistencyLevel.ALL, 1, 2); + + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedRegularTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + cluster.get(1).flush(withKeyspace("%s")); + + String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); + + long token = (Long) result.toObjectArrays()[0][0]; + + assertTrue(token < Long.parseLong(originalToken)); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(Long.toString(token - 1000)); + }); + + cluster.get(1).nodetool("cleanup", KEYSPACE, regularTableName); + + assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + }); + } +} + From b5934f8e176198acd311d85c9c6e2e4d7aa06509 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Wed, 1 Apr 2026 10:23:58 -0700 Subject: [PATCH 09/27] refactor tests --- .../accord/AccordNodetoolCleanupTest.java | 111 ++++++++---------- 1 file changed, 50 insertions(+), 61 deletions(-) diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index 89b7effe4512..3e8e7e18448e 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -18,62 +18,41 @@ package org.apache.cassandra.distributed.test.accord; -import java.io.IOException; -import java.util.Arrays; -import java.util.List; - import org.apache.cassandra.db.Keyspace; +import org.apache.cassandra.distributed.Cluster; import org.apache.cassandra.distributed.api.ConsistencyLevel; import org.apache.cassandra.distributed.api.Feature; import org.apache.cassandra.distributed.api.SimpleQueryResult; +import org.apache.cassandra.distributed.test.TestBaseImpl; import org.apache.cassandra.service.StorageService; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import static com.google.common.collect.Iterables.getOnlyElement; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.junit.jupiter.api.Assertions.assertEquals; -import org.junit.BeforeClass; import org.junit.Test; -public class AccordNodetoolCleanupTest extends AccordTestBase +public class AccordNodetoolCleanupTest extends TestBaseImpl { - private static final Logger logger = LoggerFactory.getLogger(AccordNodetoolCleanupTest.class); - - @Override - protected Logger logger() - { - return logger; - } - - @BeforeClass - public static void setupClass() throws IOException - { - AccordTestBase.setupCluster(builder -> builder - .withoutVNodes() - .withConfig(config -> - config - .set("accord.shard_durability_target_splits", "1") - .set("accord.shard_durability_cycle", "20s") - .with(Feature.NETWORK, Feature.GOSSIP)), 2); - } - @Test public void accordNodetoolCleanupTest() throws Throwable { - List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', - "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}", - "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); - test(ddls, cluster -> { - - String tableName = accordTableName; - - cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); - - SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedAccordTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + String tableName = "tbl0"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + try (Cluster cluster = init(builder().withNodes(2).withoutVNodes().withConfig((config) -> + config + .set("accord.shard_durability_target_splits", "1") + .set("accord.shard_durability_cycle", "20s") + .with(Feature.NETWORK, Feature.GOSSIP)).start())) + { + cluster.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE); + cluster.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); + cluster.schemaChange("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); + + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); + + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); cluster.get(1).flush(withKeyspace("%s")); @@ -99,25 +78,30 @@ public void accordNodetoolCleanupTest() throws Throwable fail(); } - cluster.get(1).nodetool("cleanup", KEYSPACE, accordTableName); + cluster.get(1).nodetool("cleanup", KEYSPACE, tableName); assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - }); + } } @Test public void accordNodetoolCleanupRangeInUseTest() throws Throwable { - List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', - "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}", - "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); - test(ddls, cluster -> { + String tableName = "tbl0"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + try (Cluster cluster = init(builder().withNodes(2).withoutVNodes().withConfig((config) -> + config + .set("accord.shard_durability_target_splits", "1") + .set("accord.shard_durability_cycle", "20s") + .with(Feature.NETWORK, Feature.GOSSIP)).start())) { - String tableName = accordTableName; + cluster.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE); + cluster.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); + cluster.schemaChange("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); - cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); - SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedAccordTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); cluster.get(1).flush(withKeyspace("%s")); @@ -131,26 +115,31 @@ public void accordNodetoolCleanupRangeInUseTest() throws Throwable cluster.get(1).runOnInstance(() -> StorageService.instance.move(Long.toString(token - 1000))); - String accordTableName = qualifiedAccordTableName; - - cluster.get(1).nodetool("cleanup", KEYSPACE, accordTableName); + cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - }); + } } @Test public void nodetoolCleanupForNonAccordTableTest() throws Throwable { - List ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', - "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}", - "CREATE TABLE " + qualifiedRegularTableName + " (k int PRIMARY KEY, v int)"); - test(ddls, cluster -> { - String tableName = regularTableName; + String tableName = "tbl0"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + try (Cluster cluster = init(builder().withNodes(2).withoutVNodes().withConfig((config) -> + config + .set("accord.shard_durability_target_splits", "1") + .set("accord.shard_durability_cycle", "20s") + .with(Feature.NETWORK, Feature.GOSSIP)).start())) + { + + cluster.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE); + cluster.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); + cluster.schemaChange("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int)"); - cluster.coordinator(1).execute("INSERT INTO " + qualifiedRegularTableName + " (k, v) VALUES (?, ?)", ConsistencyLevel.ALL, 1, 2); + cluster.coordinator(1).execute("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)", ConsistencyLevel.ALL, 1, 2); - SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedRegularTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); cluster.get(1).flush(withKeyspace("%s")); @@ -166,10 +155,10 @@ public void nodetoolCleanupForNonAccordTableTest() throws Throwable StorageService.instance.move(Long.toString(token - 1000)); }); - cluster.get(1).nodetool("cleanup", KEYSPACE, regularTableName); + cluster.get(1).nodetool("cleanup", KEYSPACE, tableName); assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - }); + } } } From d826bee27a92084b65597c2c00315e212781e34d Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Wed, 8 Apr 2026 15:43:19 -0700 Subject: [PATCH 10/27] Rather than checking SSTable intersection, add in use Accord Ranges to owned ranges and let existing logic handle --- .gitmodules | 4 +- .../db/compaction/CompactionManager.java | 114 +++--------------- .../service/accord/AccordService.java | 6 + .../service/accord/IAccordService.java | 14 +++ .../accord/AccordNodetoolCleanupTest.java | 52 ++++++++ 5 files changed, 92 insertions(+), 98 deletions(-) diff --git a/.gitmodules b/.gitmodules index 616dacf610a7..57ff6dc67b7f 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,4 +1,4 @@ [submodule "modules/accord"] path = modules/accord - url = https://github.com/apache/cassandra-accord.git - branch = trunk + url = https://github.com/alanwang67/cassandra-accord.git + branch = CASSANDRA-19433 diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index 336f07533d49..df72f176845b 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -39,6 +39,7 @@ import java.util.function.Predicate; import java.util.function.Supplier; import java.util.stream.Collectors; +import java.util.stream.Stream; import javax.management.openmbean.OpenDataException; import javax.management.openmbean.TabularData; @@ -65,7 +66,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import accord.primitives.AbstractRanges; import accord.primitives.Ranges; import org.apache.cassandra.cache.AutoSavingCache; @@ -117,13 +117,11 @@ import org.apache.cassandra.repair.NoSuchRepairSessionException; import org.apache.cassandra.schema.CompactionParams.TombstoneOption; import org.apache.cassandra.schema.Schema; -import org.apache.cassandra.schema.TableId; import org.apache.cassandra.schema.TableMetadata; import org.apache.cassandra.service.ActiveRepairService; import org.apache.cassandra.service.StorageService; import org.apache.cassandra.service.accord.AccordService; import org.apache.cassandra.service.accord.TokenRange; -import org.apache.cassandra.service.accord.api.TokenKey; import org.apache.cassandra.streaming.PreviewKind; import org.apache.cassandra.tcm.ClusterMetadata; import org.apache.cassandra.tcm.ownership.DataPlacement; @@ -620,11 +618,7 @@ public Object call() throws Exception assert compacting.originals().isEmpty(); logger.info("Finished {} for {}.{} successfully", operationType, keyspace, table); - // Some SSTables were not fully cleaned up because Accord is still using those ranges - if (operation.incompleteOperation()) - return AllSSTableOpStatus.INCOMPLETE; - else - return AllSSTableOpStatus.SUCCESSFUL; + return AllSSTableOpStatus.SUCCESSFUL; } finally { @@ -649,15 +643,13 @@ private static interface OneSSTableOperation Iterable filterSSTables(LifecycleTransaction transaction); void execute(LifecycleTransaction input) throws IOException; - boolean incompleteOperation(); } public enum AllSSTableOpStatus { SUCCESSFUL(0), ABORTED(1), - UNABLE_TO_CANCEL(2), - INCOMPLETE(3); + UNABLE_TO_CANCEL(2); public final int statusCode; @@ -682,12 +674,6 @@ public void execute(LifecycleTransaction input) { scrubOne(cfs, input, options, active); } - - @Override - public boolean incompleteOperation() - { - return false; - } }, jobs, OperationType.SCRUB); } @@ -716,12 +702,6 @@ public void execute(LifecycleTransaction input) { verifyOne(cfs, input.onlyOne(), options, active); } - - @Override - public boolean incompleteOperation() - { - return false; - } }, 0, OperationType.VERIFY); } @@ -787,11 +767,6 @@ public void execute(LifecycleTransaction txn) task.execute(active); } - @Override - public boolean incompleteOperation() - { - return false; - } }, jobs, OperationType.UPGRADE_SSTABLES); } @@ -822,14 +797,21 @@ public AllSSTableOpStatus performCleanup(final ColumnFamilyStore cfStore, int jo if (partitioner.getClass() == LocalPartitioner.class) localWrites = RangesAtEndpoint.of(Replica.fullReplica(local, new Range<>(partitioner.getMinimumToken(), partitioner.getMinimumToken()))); - final Set> allRanges = new HashSet<>(localWrites.ranges()); - final Set> transientRanges = new HashSet<>(localWrites.onlyTransient().ranges()); - final Set> fullRanges = new HashSet<>(localWrites.onlyFull().ranges()); - return parallelAllSSTableOperation(cfStore, new OneSSTableOperation() + Set> rangesInUseByAccord = new HashSet<>(); + if (AccordService.isSetup()) { - boolean incompleteOperation = false; + List inUseRanges = AccordService.instance().getInUseRanges(); + for (Ranges ranges : inUseRanges) + ranges.stream().forEach(r -> rangesInUseByAccord.add(((TokenRange) r).toKeyspaceRange())); + } + final Set> allRanges = Stream.concat(localWrites.ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet()); + final Set> transientRanges = Stream.concat(localWrites.onlyTransient().ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet()); + final Set> fullRanges = Stream.concat(localWrites.onlyFull().ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet()); + + return parallelAllSSTableOperation(cfStore, new OneSSTableOperation() + { @Override public Iterable filterSSTables(LifecycleTransaction transaction) { @@ -837,28 +819,15 @@ public Iterable filterSSTables(LifecycleTransaction transaction) Iterator sstableIter = sortedSSTables.iterator(); int totalSSTables = 0; int skippedSStables = 0; - int sstablesInUseByAccord = 0; while (sstableIter.hasNext()) { SSTableReader sstable = sstableIter.next(); boolean needsCleanupFull = needsCleanup(sstable, fullRanges); boolean needsCleanupTransient = !transientRanges.isEmpty() && sstable.isRepaired() && needsCleanup(sstable, transientRanges); totalSSTables++; - - if (sstableContainsRangesNeededByAccord(cfStore.getTableId(), sstable)) - { - logger.debug("Skipping {} ([{}, {}]) for cleanup; as Accord still needs the ranges.", - sstable, - sstable.getFirst().getToken(), - sstable.getLast().getToken()); - sstableIter.remove(); - transaction.cancel(sstable); - sstablesInUseByAccord++; - incompleteOperation = true; - } //If there are no ranges for which the table needs cleanup either due to lack of intersection or lack //of the table being repaired. - else if (!needsCleanupFull && !needsCleanupTransient) + if (!needsCleanupFull && !needsCleanupTransient) { logger.debug("Skipping {} ([{}, {}]) for cleanup; all rows should be kept. Needs cleanup full ranges: {} Needs cleanup transient ranges: {} Repaired: {}", sstable, @@ -873,12 +842,8 @@ else if (!needsCleanupFull && !needsCleanupTransient) } } - logger.info("Skipping cleanup for {} sstables for {}.{} since they are fully contained in owned ranges (full ranges: {}, transient ranges: {})", - skippedSStables, cfStore.getKeyspaceName(), cfStore.getTableName(), fullRanges, transientRanges); - logger.info("Skipping cleanup for {} sstables for {}.{} since they are still being used by Accord", - sstablesInUseByAccord, cfStore.getKeyspaceName(), cfStore.getTableName()); - logger.info("Cleaned up {} sstables for {}.{}", totalSSTables, cfStore.getKeyspaceName(), cfStore.getTableName()); - + logger.info("Skipping cleanup for {}/{} sstables for {}.{} since they are fully contained in owned ranges (full ranges: {}, transient ranges: {})", + skippedSStables, totalSSTables, cfStore.getKeyspaceName(), cfStore.getTableName(), fullRanges, transientRanges); sortedSSTables.sort(SSTableReader.sizeComparator); return sortedSSTables; } @@ -889,12 +854,6 @@ public void execute(LifecycleTransaction txn) throws IOException CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfStore, allRanges, transientRanges, txn.onlyOne().isRepaired(), FBUtilities.nowInSeconds()); doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes); } - - @Override - public boolean incompleteOperation() - { - return incompleteOperation; - } }, jobs, OperationType.CLEANUP); } @@ -962,12 +921,6 @@ protected int getLevel() task.setCompactionType(OperationType.GARBAGE_COLLECT); task.execute(active); } - - @Override - public boolean incompleteOperation() - { - return false; - } }, jobs, OperationType.GARBAGE_COLLECT); } @@ -1036,12 +989,6 @@ public void execute(LifecycleTransaction txn) task.setCompactionType(OperationType.RELOCATE); task.execute(active); } - - @Override - public boolean incompleteOperation() - { - return false; - } }, jobs, OperationType.RELOCATE); } @@ -1670,31 +1617,6 @@ public static boolean needsCleanup(SSTableReader sstable, Collection> futureAccordOwnedRanges = AccordService.toFuture(AccordService.instance().node().commandStores().getInUseRanges()); - logger.info("Waiting for Accord to be ready"); - - try - { - Ranges accordOwnedRanges = futureAccordOwnedRanges.get().stream().reduce(Ranges.EMPTY, (acc, r) -> acc.union(AbstractRanges.UnionMode.MERGE_ADJACENT, r)); - - if (sstable.getFirst().equals(sstable.getLast())) - return accordOwnedRanges.intersects(new TokenKey(tableId, sstable.getFirst().getToken())); - - Ranges sstableRanges = Ranges.of(TokenRange.create(tableId, sstable.getFirst().getToken(), sstable.getLast().getToken())); - return accordOwnedRanges.intersects(sstableRanges); - } - catch (Throwable e) - { - logger.error("Error while waiting for Accord in use ranges", e); - return true; - } - } - /** * This function goes over a file and removes the keys that the node is not responsible for * and only keeps keys that this node is responsible for. diff --git a/src/java/org/apache/cassandra/service/accord/AccordService.java b/src/java/org/apache/cassandra/service/accord/AccordService.java index 2c5a12ccfc82..7f75276ecc27 100644 --- a/src/java/org/apache/cassandra/service/accord/AccordService.java +++ b/src/java/org/apache/cassandra/service/accord/AccordService.java @@ -1175,6 +1175,12 @@ public void awaitDone(TableId id, long epoch) getBlocking(node.durability().sync("Drop Keyspace/Table (Epoch " + epoch + ')', ExclusiveSyncPoint, TxnId.minForEpoch(epoch), ranges, Self, All, DatabaseDescriptor.getAccordRangeSyncPointTimeoutNanos(), NANOSECONDS), ranges, new LatencyRequestBookkeeping(null), startedAt, deadline, false); } + @Override + public List getInUseRanges() + { + return getBlocking(node.commandStores().getInUseRanges()); + } + public Params journalConfiguration() { return journal.configuration(); diff --git a/src/java/org/apache/cassandra/service/accord/IAccordService.java b/src/java/org/apache/cassandra/service/accord/IAccordService.java index 4364dd95f7a0..308b846c8cbe 100644 --- a/src/java/org/apache/cassandra/service/accord/IAccordService.java +++ b/src/java/org/apache/cassandra/service/accord/IAccordService.java @@ -183,6 +183,8 @@ public AccordCompactionInfos(DurableBefore durableBefore, long minEpoch, AccordC void awaitDone(TableId id, long epoch); + List getInUseRanges(); + AccordEndpointMapper endpointMapper(); AccordTopologyService topologyService(); @@ -352,6 +354,12 @@ public void awaitDone(TableId id, long epoch) } + @Override + public List getInUseRanges() + { + return List.of(Ranges.EMPTY); + } + @Override public AccordEndpointMapper endpointMapper() { @@ -562,6 +570,12 @@ public void awaitDone(TableId id, long epoch) delegate.awaitDone(id, epoch); } + @Override + public List getInUseRanges() + { + return delegate.getInUseRanges(); + } + @Override public Params journalConfiguration() { diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index 3e8e7e18448e..73564229b2a6 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -84,6 +84,58 @@ public void accordNodetoolCleanupTest() throws Throwable } } + @Test + public void accordNodetoolCleanupPartialSSTableTest() throws Throwable + { + String tableName = "tbl0"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + try (Cluster cluster = init(builder().withNodes(2).withoutVNodes().withConfig((config) -> + config + .set("accord.shard_durability_target_splits", "1") + .set("accord.shard_durability_cycle", "20s") + .with(Feature.NETWORK, Feature.GOSSIP)).start())) + { + cluster.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE); + cluster.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); + cluster.schemaChange("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); + + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 2, 2); + + SimpleQueryResult result1 = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 2 LIMIT 1", ConsistencyLevel.SERIAL); + SimpleQueryResult result2 = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + cluster.get(1).flush(withKeyspace("%s")); + + String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); + + long token1 = (Long) result1.toObjectArrays()[0][0]; + long token2 = (Long) result2.toObjectArrays()[0][0]; + + assertTrue(token2 < token1 && token1 < Long.parseLong(originalToken)); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(Long.toString(token1 - 1)); + }); + + // Wait until Accord retires range + try + { + Thread.sleep(20000); + } + catch (InterruptedException e) + { + fail(); + } + + cluster.get(1).nodetool("cleanup", KEYSPACE, tableName); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + } + } + @Test public void accordNodetoolCleanupRangeInUseTest() throws Throwable { From 804e6dda7b7bc8c0522c43577955e8bf256b6cbf Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Wed, 8 Apr 2026 16:31:42 -0700 Subject: [PATCH 11/27] added incomplete operation --- .../db/compaction/CompactionManager.java | 87 ++++++++++++++++--- 1 file changed, 74 insertions(+), 13 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index df72f176845b..c5a42b754a5c 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -618,7 +618,13 @@ public Object call() throws Exception assert compacting.originals().isEmpty(); logger.info("Finished {} for {}.{} successfully", operationType, keyspace, table); - return AllSSTableOpStatus.SUCCESSFUL; + if (operation.incompleteOperation()) + { + logger.info("Operation incomplete for {}.{}", keyspace, table); + return AllSSTableOpStatus.INCOMPLETE; + } + else + return AllSSTableOpStatus.SUCCESSFUL; } finally { @@ -643,13 +649,15 @@ private static interface OneSSTableOperation Iterable filterSSTables(LifecycleTransaction transaction); void execute(LifecycleTransaction input) throws IOException; + boolean incompleteOperation(); } public enum AllSSTableOpStatus { SUCCESSFUL(0), ABORTED(1), - UNABLE_TO_CANCEL(2); + UNABLE_TO_CANCEL(2), + INCOMPLETE(3); public final int statusCode; @@ -674,6 +682,12 @@ public void execute(LifecycleTransaction input) { scrubOne(cfs, input, options, active); } + + @Override + public boolean incompleteOperation() + { + return false; + } }, jobs, OperationType.SCRUB); } @@ -702,6 +716,12 @@ public void execute(LifecycleTransaction input) { verifyOne(cfs, input.onlyOne(), options, active); } + + @Override + public boolean incompleteOperation() + { + return false; + } }, 0, OperationType.VERIFY); } @@ -767,6 +787,11 @@ public void execute(LifecycleTransaction txn) task.execute(active); } + @Override + public boolean incompleteOperation() + { + return false; + } }, jobs, OperationType.UPGRADE_SSTABLES); } @@ -797,7 +822,6 @@ public AllSSTableOpStatus performCleanup(final ColumnFamilyStore cfStore, int jo if (partitioner.getClass() == LocalPartitioner.class) localWrites = RangesAtEndpoint.of(Replica.fullReplica(local, new Range<>(partitioner.getMinimumToken(), partitioner.getMinimumToken()))); - Set> rangesInUseByAccord = new HashSet<>(); if (AccordService.isSetup()) { @@ -812,6 +836,8 @@ public AllSSTableOpStatus performCleanup(final ColumnFamilyStore cfStore, int jo return parallelAllSSTableOperation(cfStore, new OneSSTableOperation() { + boolean incompleteOperation; + @Override public Iterable filterSSTables(LifecycleTransaction transaction) { @@ -852,7 +878,13 @@ public Iterable filterSSTables(LifecycleTransaction transaction) public void execute(LifecycleTransaction txn) throws IOException { CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfStore, allRanges, transientRanges, txn.onlyOne().isRepaired(), FBUtilities.nowInSeconds()); - doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes); + this.incompleteOperation = doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes, rangesInUseByAccord); + } + + @Override + public boolean incompleteOperation() + { + return incompleteOperation; } }, jobs, OperationType.CLEANUP); } @@ -921,6 +953,12 @@ protected int getLevel() task.setCompactionType(OperationType.GARBAGE_COLLECT); task.execute(active); } + + @Override + public boolean incompleteOperation() + { + return false; + } }, jobs, OperationType.GARBAGE_COLLECT); } @@ -989,6 +1027,12 @@ public void execute(LifecycleTransaction txn) task.setCompactionType(OperationType.RELOCATE); task.execute(active); } + + @Override + public boolean incompleteOperation() + { + return false; + } }, jobs, OperationType.RELOCATE); } @@ -1409,8 +1453,18 @@ public void forceUserDefinedCleanup(String dataFiles) ColumnFamilyStore cfs = entry.getKey(); Keyspace keyspace = cfs.keyspace; final RangesAtEndpoint replicas = StorageService.instance.getLocalReplicas(keyspace.getName()); - final Set> allRanges = replicas.ranges(); - final Set> transientRanges = replicas.onlyTransient().ranges(); + + Set> rangesInUseByAccord = new HashSet<>(); + if (AccordService.isSetup()) + { + List inUseRanges = AccordService.instance().getInUseRanges(); + for (Ranges ranges : inUseRanges) + ranges.stream().forEach(r -> rangesInUseByAccord.add(((TokenRange) r).toKeyspaceRange())); + } + + final Set> allRanges = Stream.concat(replicas.ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet()); + final Set> transientRanges = Stream.concat(replicas.onlyTransient().ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet()); + boolean hasIndexes = cfs.indexManager.hasIndexes(); SSTableReader sstable = lookupSSTable(cfs, entry.getValue()); @@ -1423,7 +1477,7 @@ public void forceUserDefinedCleanup(String dataFiles) CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfs, allRanges, transientRanges, sstable.isRepaired(), FBUtilities.nowInSeconds()); try (LifecycleTransaction txn = cfs.getTracker().tryModify(sstable, OperationType.CLEANUP)) { - doCleanupOne(cfs, txn, cleanupStrategy, allRanges, hasIndexes); + doCleanupOne(cfs, txn, cleanupStrategy, allRanges, hasIndexes, rangesInUseByAccord); } catch (IOException e) { @@ -1623,11 +1677,12 @@ public static boolean needsCleanup(SSTableReader sstable, Collection> allRanges, - boolean hasIndexes) throws IOException + private boolean doCleanupOne(final ColumnFamilyStore cfs, + LifecycleTransaction txn, + CleanupStrategy cleanupStrategy, + Collection> allRanges, + boolean hasIndexes, + Collection> accordOwnedRanges) throws IOException { assert !cfs.isIndex(); @@ -1639,9 +1694,10 @@ private void doCleanupOne(final ColumnFamilyStore cfs, txn.obsoleteOriginals(); txn.finish(); logger.info("SSTable {} ([{}, {}]) does not intersect the owned ranges ({}), dropping it", sstable, sstable.getFirst().getToken(), sstable.getLast().getToken(), allRanges); - return; + return false; } + boolean incomplete = false; long start = nanoTime(); long totalkeysWritten = 0; @@ -1680,6 +1736,9 @@ private void doCleanupOne(final ColumnFamilyStore cfs, if (notCleaned == null) continue; + if (Range.isInRanges(partition.partitionKey().getToken(), accordOwnedRanges)) + incomplete = true; + if (writer.append(notCleaned) != null) totalkeysWritten++; @@ -1710,6 +1769,8 @@ private void doCleanupOne(final ColumnFamilyStore cfs, FBUtilities.prettyPrintMemory(endsize), (int) (ratio * 100), totalkeysWritten, dTime)); } + return incomplete; + } protected void compactionRateLimiterAcquire(RateLimiter limiter, long bytesScanned, long lastBytesScanned, double compressionRatio) From 5742a4cf8745b24d80c9ffd4ae0b4e8d0157dc82 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Sun, 12 Apr 2026 22:38:01 -0700 Subject: [PATCH 12/27] Handle nodeprobe + add check for transient range + change inUseRanges signature --- .../db/compaction/CompactionManager.java | 115 +++++++++--------- .../apache/cassandra/repair/Validator.java | 2 + .../service/accord/AccordService.java | 25 +++- .../service/accord/IAccordService.java | 13 +- .../org/apache/cassandra/tools/NodeProbe.java | 3 + .../accord/AccordNodetoolCleanupTest.java | 31 ++--- 6 files changed, 108 insertions(+), 81 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index c5a42b754a5c..f9e229ffb2f1 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -66,8 +66,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import accord.primitives.Ranges; - import org.apache.cassandra.cache.AutoSavingCache; import org.apache.cassandra.concurrent.ExecutorFactory; import org.apache.cassandra.concurrent.WrappedExecutorPlus; @@ -121,7 +119,6 @@ import org.apache.cassandra.service.ActiveRepairService; import org.apache.cassandra.service.StorageService; import org.apache.cassandra.service.accord.AccordService; -import org.apache.cassandra.service.accord.TokenRange; import org.apache.cassandra.streaming.PreviewKind; import org.apache.cassandra.tcm.ClusterMetadata; import org.apache.cassandra.tcm.ownership.DataPlacement; @@ -585,8 +582,16 @@ private AllSSTableOpStatus parallelAllSSTableOperation(final ColumnFamilyStore c Iterable sstables = Lists.newArrayList(operation.filterSSTables(compacting)); if (Iterables.isEmpty(sstables)) { - logger.info("No sstables to {} for {}.{}", operationName, keyspace, table); - return AllSSTableOpStatus.SUCCESSFUL; + if (operation.incompleteOperation()) + { + logger.info("Operation incomplete for {}.{}", keyspace, table); + return AllSSTableOpStatus.INCOMPLETE; + } + else + { + logger.info("No sstables to {} for {}.{}", operationName, keyspace, table); + return AllSSTableOpStatus.SUCCESSFUL; + } } for (final SSTableReader sstable : sstables) @@ -649,7 +654,9 @@ private static interface OneSSTableOperation Iterable filterSSTables(LifecycleTransaction transaction); void execute(LifecycleTransaction input) throws IOException; - boolean incompleteOperation(); + default boolean incompleteOperation() { + return false; + }; } public enum AllSSTableOpStatus @@ -682,12 +689,6 @@ public void execute(LifecycleTransaction input) { scrubOne(cfs, input, options, active); } - - @Override - public boolean incompleteOperation() - { - return false; - } }, jobs, OperationType.SCRUB); } @@ -716,12 +717,6 @@ public void execute(LifecycleTransaction input) { verifyOne(cfs, input.onlyOne(), options, active); } - - @Override - public boolean incompleteOperation() - { - return false; - } }, 0, OperationType.VERIFY); } @@ -786,12 +781,6 @@ public void execute(LifecycleTransaction txn) task.setCompactionType(OperationType.UPGRADE_SSTABLES); task.execute(active); } - - @Override - public boolean incompleteOperation() - { - return false; - } }, jobs, OperationType.UPGRADE_SSTABLES); } @@ -822,17 +811,26 @@ public AllSSTableOpStatus performCleanup(final ColumnFamilyStore cfStore, int jo if (partitioner.getClass() == LocalPartitioner.class) localWrites = RangesAtEndpoint.of(Replica.fullReplica(local, new Range<>(partitioner.getMinimumToken(), partitioner.getMinimumToken()))); - Set> rangesInUseByAccord = new HashSet<>(); + Set> noLongerOwnedRangesInUseByAccord = new HashSet<>(); if (AccordService.isSetup()) { - List inUseRanges = AccordService.instance().getInUseRanges(); - for (Ranges ranges : inUseRanges) - ranges.stream().forEach(r -> rangesInUseByAccord.add(((TokenRange) r).toKeyspaceRange())); + if (!localWrites.onlyTransient().ranges().isEmpty()) + { + logger.error("Transient replication is not supported for Accord"); + return AllSSTableOpStatus.ABORTED; + } + + if (AccordService.instance().getInUseRanges().containsKey(cfStore.getTableId())) + { + Set> accordOwnedRanges = AccordService.instance().getInUseRanges().get(cfStore.getTableId()); + for (Range range : accordOwnedRanges) + noLongerOwnedRangesInUseByAccord.addAll(range.subtractAll(localWrites.ranges())); + } } - final Set> allRanges = Stream.concat(localWrites.ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet()); - final Set> transientRanges = Stream.concat(localWrites.onlyTransient().ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet()); - final Set> fullRanges = Stream.concat(localWrites.onlyFull().ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet()); + final Set> allRanges = Stream.concat(localWrites.ranges().stream(), noLongerOwnedRangesInUseByAccord.stream()).collect(Collectors.toSet()); + final Set> transientRanges = new HashSet<>(localWrites.onlyTransient().ranges()); + final Set> fullRanges = Stream.concat(localWrites.onlyFull().ranges().stream(), noLongerOwnedRangesInUseByAccord.stream()).collect(Collectors.toSet()); return parallelAllSSTableOperation(cfStore, new OneSSTableOperation() { @@ -850,9 +848,9 @@ public Iterable filterSSTables(LifecycleTransaction transaction) SSTableReader sstable = sstableIter.next(); boolean needsCleanupFull = needsCleanup(sstable, fullRanges); boolean needsCleanupTransient = !transientRanges.isEmpty() && sstable.isRepaired() && needsCleanup(sstable, transientRanges); - totalSSTables++; //If there are no ranges for which the table needs cleanup either due to lack of intersection or lack //of the table being repaired. + totalSSTables++; if (!needsCleanupFull && !needsCleanupTransient) { logger.debug("Skipping {} ([{}, {}]) for cleanup; all rows should be kept. Needs cleanup full ranges: {} Needs cleanup transient ranges: {} Repaired: {}", @@ -866,6 +864,15 @@ public Iterable filterSSTables(LifecycleTransaction transaction) transaction.cancel(sstable); skippedSStables++; } + + for (Range range : noLongerOwnedRangesInUseByAccord) + { + if (range.intersects(new Range<>(sstable.getFirst().getToken(), sstable.getLast().getToken()))) + { + this.incompleteOperation = true; + break; + } + } } logger.info("Skipping cleanup for {}/{} sstables for {}.{} since they are fully contained in owned ranges (full ranges: {}, transient ranges: {})", @@ -878,7 +885,7 @@ public Iterable filterSSTables(LifecycleTransaction transaction) public void execute(LifecycleTransaction txn) throws IOException { CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfStore, allRanges, transientRanges, txn.onlyOne().isRepaired(), FBUtilities.nowInSeconds()); - this.incompleteOperation = doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes, rangesInUseByAccord); + doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes); } @Override @@ -953,12 +960,6 @@ protected int getLevel() task.setCompactionType(OperationType.GARBAGE_COLLECT); task.execute(active); } - - @Override - public boolean incompleteOperation() - { - return false; - } }, jobs, OperationType.GARBAGE_COLLECT); } @@ -1027,12 +1028,6 @@ public void execute(LifecycleTransaction txn) task.setCompactionType(OperationType.RELOCATE); task.execute(active); } - - @Override - public boolean incompleteOperation() - { - return false; - } }, jobs, OperationType.RELOCATE); } @@ -1454,16 +1449,25 @@ public void forceUserDefinedCleanup(String dataFiles) Keyspace keyspace = cfs.keyspace; final RangesAtEndpoint replicas = StorageService.instance.getLocalReplicas(keyspace.getName()); - Set> rangesInUseByAccord = new HashSet<>(); + Set> noLongerOwnedRangesInUseByAccord = new HashSet<>(); if (AccordService.isSetup()) { - List inUseRanges = AccordService.instance().getInUseRanges(); - for (Ranges ranges : inUseRanges) - ranges.stream().forEach(r -> rangesInUseByAccord.add(((TokenRange) r).toKeyspaceRange())); + if (!replicas.onlyTransient().ranges().isEmpty()) + { + logger.error("Transient replication is not supported for Accord"); + return; + } + + if (AccordService.instance().getInUseRanges().containsKey(cfs.getTableId())) + { + Set> accordOwnedRanges = AccordService.instance().getInUseRanges().get(cfs.getTableId()); + for (Range range : accordOwnedRanges) + noLongerOwnedRangesInUseByAccord.addAll(range.subtractAll(replicas.ranges())); + } } - final Set> allRanges = Stream.concat(replicas.ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet()); - final Set> transientRanges = Stream.concat(replicas.onlyTransient().ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet()); + final Set> allRanges = Stream.concat(replicas.ranges().stream(), noLongerOwnedRangesInUseByAccord.stream()).collect(Collectors.toSet()); + final Set> transientRanges = new HashSet<>(replicas.onlyTransient().ranges()); boolean hasIndexes = cfs.indexManager.hasIndexes(); SSTableReader sstable = lookupSSTable(cfs, entry.getValue()); @@ -1477,7 +1481,7 @@ public void forceUserDefinedCleanup(String dataFiles) CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfs, allRanges, transientRanges, sstable.isRepaired(), FBUtilities.nowInSeconds()); try (LifecycleTransaction txn = cfs.getTracker().tryModify(sstable, OperationType.CLEANUP)) { - doCleanupOne(cfs, txn, cleanupStrategy, allRanges, hasIndexes, rangesInUseByAccord); + doCleanupOne(cfs, txn, cleanupStrategy, allRanges, hasIndexes); } catch (IOException e) { @@ -1681,8 +1685,8 @@ private boolean doCleanupOne(final ColumnFamilyStore cfs, LifecycleTransaction txn, CleanupStrategy cleanupStrategy, Collection> allRanges, - boolean hasIndexes, - Collection> accordOwnedRanges) throws IOException + boolean hasIndexes + ) throws IOException { assert !cfs.isIndex(); @@ -1736,9 +1740,6 @@ private boolean doCleanupOne(final ColumnFamilyStore cfs, if (notCleaned == null) continue; - if (Range.isInRanges(partition.partitionKey().getToken(), accordOwnedRanges)) - incomplete = true; - if (writer.append(notCleaned) != null) totalkeysWritten++; diff --git a/src/java/org/apache/cassandra/repair/Validator.java b/src/java/org/apache/cassandra/repair/Validator.java index c1147f4a6072..362905f53c1c 100644 --- a/src/java/org/apache/cassandra/repair/Validator.java +++ b/src/java/org/apache/cassandra/repair/Validator.java @@ -192,7 +192,9 @@ public void add(UnfilteredRowIterator partition) if (rowHash != null) { if(topPartitionCollector != null) + { topPartitionCollector.trackPartitionSize(partition.partitionKey(), rowHash.size); + } range.addHash(rowHash); } } diff --git a/src/java/org/apache/cassandra/service/accord/AccordService.java b/src/java/org/apache/cassandra/service/accord/AccordService.java index 7f75276ecc27..a3b6362d2603 100644 --- a/src/java/org/apache/cassandra/service/accord/AccordService.java +++ b/src/java/org/apache/cassandra/service/accord/AccordService.java @@ -21,8 +21,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; @@ -87,6 +89,7 @@ import org.apache.cassandra.db.ConsistencyLevel; import org.apache.cassandra.db.SystemKeyspace; import org.apache.cassandra.db.SystemKeyspace.BootstrapState; +import org.apache.cassandra.dht.Token; import org.apache.cassandra.exceptions.RequestExecutionException; import org.apache.cassandra.journal.Params; import org.apache.cassandra.locator.InetAddressAndPort; @@ -1176,9 +1179,27 @@ public void awaitDone(TableId id, long epoch) } @Override - public List getInUseRanges() + public Map>> getInUseRanges() { - return getBlocking(node.commandStores().getInUseRanges()); + Map>> tableToRangeMap = new HashMap<>(); + List globalRanges = getBlocking(node.commandStores().getInUseRanges()); + + for (Ranges ranges : globalRanges) + { + ranges.stream().forEach(r -> { + TokenRange tokenRange = (TokenRange) r; + if (tableToRangeMap.containsKey(tokenRange.table())) + { + tableToRangeMap.get(tokenRange.table()).add(((TokenRange) r).toKeyspaceRange()); + } + else + { + tableToRangeMap.put(tokenRange.table(), new HashSet<>()); + } + }); + } + + return tableToRangeMap; } public Params journalConfiguration() diff --git a/src/java/org/apache/cassandra/service/accord/IAccordService.java b/src/java/org/apache/cassandra/service/accord/IAccordService.java index 308b846c8cbe..9349e9a63afe 100644 --- a/src/java/org/apache/cassandra/service/accord/IAccordService.java +++ b/src/java/org/apache/cassandra/service/accord/IAccordService.java @@ -20,7 +20,10 @@ import java.util.Collection; import java.util.EnumSet; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.function.BiConsumer; @@ -55,6 +58,8 @@ import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.db.ConsistencyLevel; +import org.apache.cassandra.dht.Range; +import org.apache.cassandra.dht.Token; import org.apache.cassandra.exceptions.RequestExecutionException; import org.apache.cassandra.journal.Params; import org.apache.cassandra.net.IVerbHandler; @@ -183,7 +188,7 @@ public AccordCompactionInfos(DurableBefore durableBefore, long minEpoch, AccordC void awaitDone(TableId id, long epoch); - List getInUseRanges(); + Map>> getInUseRanges(); AccordEndpointMapper endpointMapper(); @@ -355,9 +360,9 @@ public void awaitDone(TableId id, long epoch) } @Override - public List getInUseRanges() + public Map>> getInUseRanges() { - return List.of(Ranges.EMPTY); + return new HashMap<>(); } @Override @@ -571,7 +576,7 @@ public void awaitDone(TableId id, long epoch) } @Override - public List getInUseRanges() + public Map>> getInUseRanges() { return delegate.getInUseRanges(); } diff --git a/src/java/org/apache/cassandra/tools/NodeProbe.java b/src/java/org/apache/cassandra/tools/NodeProbe.java index 567df0928342..ae5599865a55 100644 --- a/src/java/org/apache/cassandra/tools/NodeProbe.java +++ b/src/java/org/apache/cassandra/tools/NodeProbe.java @@ -480,6 +480,9 @@ private void perform(PrintStream out, String ks, Job job, String jobName) throws case 2: out.printf("Failed marking some sstables compacting in keyspace %s, check server logs for more information.\n", ks); throw new RuntimeException(String.format("Failed marking some sstables compacting in keyspace %s, check server logs for more information.\n", ks)); + case 3: + out.printf("Partially cleaned up SSTables for ranges that are no longer owned in keyspace %s, check server logs for more information.\n", ks); + throw new RuntimeException(String.format("Partially cleaned up SSTables for ranges that are no longer owned in keyspace %s, check server logs for more information.\n", ks)); } } diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index 73564229b2a6..8e99322dad72 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -22,6 +22,7 @@ import org.apache.cassandra.distributed.Cluster; import org.apache.cassandra.distributed.api.ConsistencyLevel; import org.apache.cassandra.distributed.api.Feature; +import org.apache.cassandra.distributed.api.NodeToolResult; import org.apache.cassandra.distributed.api.SimpleQueryResult; import org.apache.cassandra.distributed.test.TestBaseImpl; import org.apache.cassandra.service.StorageService; @@ -30,7 +31,6 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.junit.jupiter.api.Assertions.assertEquals; - import org.junit.Test; public class AccordNodetoolCleanupTest extends TestBaseImpl @@ -64,11 +64,12 @@ public void accordNodetoolCleanupTest() throws Throwable assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + // Cluster 1 no longer owns token cluster.get(1).runOnInstance(() -> { StorageService.instance.move(Long.toString(token - 1000)); }); - // Wait until Accord retires range + // Wait until Accord retires range, so it no longer has ownership of token try { Thread.sleep(20000); @@ -112,25 +113,20 @@ public void accordNodetoolCleanupPartialSSTableTest() throws Throwable long token1 = (Long) result1.toObjectArrays()[0][0]; long token2 = (Long) result2.toObjectArrays()[0][0]; - assertTrue(token2 < token1 && token1 < Long.parseLong(originalToken)); + assertTrue((token2 < (token1 - 1000)) && token1 < Long.parseLong(originalToken)); assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + // Cluster 1 now only owns token2 cluster.get(1).runOnInstance(() -> { - StorageService.instance.move(Long.toString(token1 - 1)); + StorageService.instance.move(Long.toString(token1 - 1000)); }); - // Wait until Accord retires range - try - { - Thread.sleep(20000); - } - catch (InterruptedException e) - { - fail(); - } + assertEquals(cluster.get(1).callOnInstance(() -> Long.parseLong(getOnlyElement(StorageService.instance.getTokens()))), token1 - 1000); - cluster.get(1).nodetool("cleanup", KEYSPACE, tableName); + NodeToolResult result = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); + + assertTrue(result.getStdout().contains("Partially cleaned up SSTables for ranges that are no longer owned in keyspace " + KEYSPACE)); assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); } @@ -143,10 +139,8 @@ public void accordNodetoolCleanupRangeInUseTest() throws Throwable String qualifiedTableName = KEYSPACE + '.' + tableName; try (Cluster cluster = init(builder().withNodes(2).withoutVNodes().withConfig((config) -> config - .set("accord.shard_durability_target_splits", "1") - .set("accord.shard_durability_cycle", "20s") - .with(Feature.NETWORK, Feature.GOSSIP)).start())) { - + .with(Feature.NETWORK, Feature.GOSSIP)).start())) + { cluster.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE); cluster.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); cluster.schemaChange("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); @@ -169,6 +163,7 @@ public void accordNodetoolCleanupRangeInUseTest() throws Throwable cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); + // Cluster 1 no longer owns token, however Accord still needs it so it is no cleaned up assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); } } From 6e55ae182de2d0eb7f794f7c633ccbb7f4010b55 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Sun, 12 Apr 2026 22:40:45 -0700 Subject: [PATCH 13/27] remove new line --- .../org/apache/cassandra/db/compaction/CompactionManager.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index f9e229ffb2f1..d16718bf4b8c 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -651,7 +651,6 @@ public Object call() throws Exception private static interface OneSSTableOperation { - Iterable filterSSTables(LifecycleTransaction transaction); void execute(LifecycleTransaction input) throws IOException; default boolean incompleteOperation() { From b0311b83ea8dfd87cd4aba867d2271e45ee56879 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Sun, 12 Apr 2026 22:48:00 -0700 Subject: [PATCH 14/27] fix bug --- .../cassandra/db/compaction/CompactionManager.java | 10 +++------- src/java/org/apache/cassandra/repair/Validator.java | 2 -- .../apache/cassandra/service/accord/AccordService.java | 10 +++------- 3 files changed, 6 insertions(+), 16 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index d16718bf4b8c..fe05a5e629f1 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -1680,12 +1680,11 @@ public static boolean needsCleanup(SSTableReader sstable, Collection> allRanges, - boolean hasIndexes - ) throws IOException + boolean hasIndexes) throws IOException { assert !cfs.isIndex(); @@ -1697,10 +1696,9 @@ private boolean doCleanupOne(final ColumnFamilyStore cfs, txn.obsoleteOriginals(); txn.finish(); logger.info("SSTable {} ([{}, {}]) does not intersect the owned ranges ({}), dropping it", sstable, sstable.getFirst().getToken(), sstable.getLast().getToken(), allRanges); - return false; + return; } - boolean incomplete = false; long start = nanoTime(); long totalkeysWritten = 0; @@ -1769,8 +1767,6 @@ private boolean doCleanupOne(final ColumnFamilyStore cfs, FBUtilities.prettyPrintMemory(endsize), (int) (ratio * 100), totalkeysWritten, dTime)); } - return incomplete; - } protected void compactionRateLimiterAcquire(RateLimiter limiter, long bytesScanned, long lastBytesScanned, double compressionRatio) diff --git a/src/java/org/apache/cassandra/repair/Validator.java b/src/java/org/apache/cassandra/repair/Validator.java index 362905f53c1c..c1147f4a6072 100644 --- a/src/java/org/apache/cassandra/repair/Validator.java +++ b/src/java/org/apache/cassandra/repair/Validator.java @@ -192,9 +192,7 @@ public void add(UnfilteredRowIterator partition) if (rowHash != null) { if(topPartitionCollector != null) - { topPartitionCollector.trackPartitionSize(partition.partitionKey(), rowHash.size); - } range.addHash(rowHash); } } diff --git a/src/java/org/apache/cassandra/service/accord/AccordService.java b/src/java/org/apache/cassandra/service/accord/AccordService.java index a3b6362d2603..e0859c3398cb 100644 --- a/src/java/org/apache/cassandra/service/accord/AccordService.java +++ b/src/java/org/apache/cassandra/service/accord/AccordService.java @@ -1188,14 +1188,10 @@ public Map>> getInUseRanges() { ranges.stream().forEach(r -> { TokenRange tokenRange = (TokenRange) r; - if (tableToRangeMap.containsKey(tokenRange.table())) - { - tableToRangeMap.get(tokenRange.table()).add(((TokenRange) r).toKeyspaceRange()); - } - else - { + if (!tableToRangeMap.containsKey(tokenRange.table())) tableToRangeMap.put(tokenRange.table(), new HashSet<>()); - } + + tableToRangeMap.get(tokenRange.table()).add(tokenRange.toKeyspaceRange()); }); } From 564e22770cbd0147d448cd4c6b0bb6d2996f5117 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Mon, 13 Apr 2026 12:30:55 -0700 Subject: [PATCH 15/27] set incompleteOperation based off iterating SSTables --- .../db/compaction/CompactionManager.java | 52 ++++++++----------- .../accord/AccordNodetoolCleanupTest.java | 7 ++- 2 files changed, 25 insertions(+), 34 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index fe05a5e629f1..7d85c92aba6c 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -33,6 +33,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BooleanSupplier; @@ -582,16 +583,8 @@ private AllSSTableOpStatus parallelAllSSTableOperation(final ColumnFamilyStore c Iterable sstables = Lists.newArrayList(operation.filterSSTables(compacting)); if (Iterables.isEmpty(sstables)) { - if (operation.incompleteOperation()) - { - logger.info("Operation incomplete for {}.{}", keyspace, table); - return AllSSTableOpStatus.INCOMPLETE; - } - else - { - logger.info("No sstables to {} for {}.{}", operationName, keyspace, table); - return AllSSTableOpStatus.SUCCESSFUL; - } + logger.info("No sstables to {} for {}.{}", operationName, keyspace, table); + return AllSSTableOpStatus.SUCCESSFUL; } for (final SSTableReader sstable : sstables) @@ -829,11 +822,11 @@ public AllSSTableOpStatus performCleanup(final ColumnFamilyStore cfStore, int jo final Set> allRanges = Stream.concat(localWrites.ranges().stream(), noLongerOwnedRangesInUseByAccord.stream()).collect(Collectors.toSet()); final Set> transientRanges = new HashSet<>(localWrites.onlyTransient().ranges()); - final Set> fullRanges = Stream.concat(localWrites.onlyFull().ranges().stream(), noLongerOwnedRangesInUseByAccord.stream()).collect(Collectors.toSet()); + final Set> fullRanges = new HashSet<>(localWrites.onlyFull().ranges()); return parallelAllSSTableOperation(cfStore, new OneSSTableOperation() { - boolean incompleteOperation; + final AtomicBoolean incompleteOperation = new AtomicBoolean(false); @Override public Iterable filterSSTables(LifecycleTransaction transaction) @@ -863,15 +856,6 @@ public Iterable filterSSTables(LifecycleTransaction transaction) transaction.cancel(sstable); skippedSStables++; } - - for (Range range : noLongerOwnedRangesInUseByAccord) - { - if (range.intersects(new Range<>(sstable.getFirst().getToken(), sstable.getLast().getToken()))) - { - this.incompleteOperation = true; - break; - } - } } logger.info("Skipping cleanup for {}/{} sstables for {}.{} since they are fully contained in owned ranges (full ranges: {}, transient ranges: {})", @@ -884,13 +868,15 @@ public Iterable filterSSTables(LifecycleTransaction transaction) public void execute(LifecycleTransaction txn) throws IOException { CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfStore, allRanges, transientRanges, txn.onlyOne().isRepaired(), FBUtilities.nowInSeconds()); - doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes); + boolean incompleteOperation = doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes, noLongerOwnedRangesInUseByAccord); + if (!this.incompleteOperation.get() && incompleteOperation) + this.incompleteOperation.set(true); } @Override public boolean incompleteOperation() { - return incompleteOperation; + return this.incompleteOperation.get(); } }, jobs, OperationType.CLEANUP); } @@ -1480,7 +1466,7 @@ public void forceUserDefinedCleanup(String dataFiles) CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfs, allRanges, transientRanges, sstable.isRepaired(), FBUtilities.nowInSeconds()); try (LifecycleTransaction txn = cfs.getTracker().tryModify(sstable, OperationType.CLEANUP)) { - doCleanupOne(cfs, txn, cleanupStrategy, allRanges, hasIndexes); + doCleanupOne(cfs, txn, cleanupStrategy, allRanges, hasIndexes, noLongerOwnedRangesInUseByAccord); } catch (IOException e) { @@ -1680,13 +1666,15 @@ public static boolean needsCleanup(SSTableReader sstable, Collection> allRanges, - boolean hasIndexes) throws IOException + private boolean doCleanupOne(final ColumnFamilyStore cfs, + LifecycleTransaction txn, + CleanupStrategy cleanupStrategy, + Collection> allRanges, + boolean hasIndexes, + Collection> noLongerOwnedRangesInUseByAccord) throws IOException { assert !cfs.isIndex(); + boolean incompleteOperation = false; SSTableReader sstable = txn.onlyOne(); @@ -1696,7 +1684,7 @@ private void doCleanupOne(final ColumnFamilyStore cfs, txn.obsoleteOriginals(); txn.finish(); logger.info("SSTable {} ([{}, {}]) does not intersect the owned ranges ({}), dropping it", sstable, sstable.getFirst().getToken(), sstable.getLast().getToken(), allRanges); - return; + return incompleteOperation; } long start = nanoTime(); @@ -1737,6 +1725,9 @@ private void doCleanupOne(final ColumnFamilyStore cfs, if (notCleaned == null) continue; + if (Range.isInRanges(partition.partitionKey().getToken(), noLongerOwnedRangesInUseByAccord)) + incompleteOperation = true; + if (writer.append(notCleaned) != null) totalkeysWritten++; @@ -1767,6 +1758,7 @@ private void doCleanupOne(final ColumnFamilyStore cfs, FBUtilities.prettyPrintMemory(endsize), (int) (ratio * 100), totalkeysWritten, dTime)); } + return incompleteOperation; } protected void compactionRateLimiterAcquire(RateLimiter limiter, long bytesScanned, long lastBytesScanned, double compressionRatio) diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index 8e99322dad72..3a6eeedbe949 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -29,8 +29,8 @@ import static com.google.common.collect.Iterables.getOnlyElement; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; -import static org.junit.jupiter.api.Assertions.assertEquals; import org.junit.Test; public class AccordNodetoolCleanupTest extends TestBaseImpl @@ -117,13 +117,12 @@ public void accordNodetoolCleanupPartialSSTableTest() throws Throwable assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - // Cluster 1 now only owns token2 + // Cluster 1 now only owns token2, but Accord still requires token1 cluster.get(1).runOnInstance(() -> { + Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).disableAutoCompaction(); StorageService.instance.move(Long.toString(token1 - 1000)); }); - assertEquals(cluster.get(1).callOnInstance(() -> Long.parseLong(getOnlyElement(StorageService.instance.getTokens()))), token1 - 1000); - NodeToolResult result = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); assertTrue(result.getStdout().contains("Partially cleaned up SSTables for ranges that are no longer owned in keyspace " + KEYSPACE)); From cc5c8a12aca8bf1faeb0385ac6bcc3a430cb3727 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Mon, 13 Apr 2026 12:44:34 -0700 Subject: [PATCH 16/27] removed lock --- .../cassandra/db/compaction/CompactionManager.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index 7d85c92aba6c..26760188eed7 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -33,7 +33,6 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BooleanSupplier; @@ -826,7 +825,7 @@ public AllSSTableOpStatus performCleanup(final ColumnFamilyStore cfStore, int jo return parallelAllSSTableOperation(cfStore, new OneSSTableOperation() { - final AtomicBoolean incompleteOperation = new AtomicBoolean(false); + boolean incompleteOperation = false; @Override public Iterable filterSSTables(LifecycleTransaction transaction) @@ -868,15 +867,14 @@ public Iterable filterSSTables(LifecycleTransaction transaction) public void execute(LifecycleTransaction txn) throws IOException { CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfStore, allRanges, transientRanges, txn.onlyOne().isRepaired(), FBUtilities.nowInSeconds()); - boolean incompleteOperation = doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes, noLongerOwnedRangesInUseByAccord); - if (!this.incompleteOperation.get() && incompleteOperation) - this.incompleteOperation.set(true); + if (doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes, noLongerOwnedRangesInUseByAccord)) + this.incompleteOperation = true; } @Override public boolean incompleteOperation() { - return this.incompleteOperation.get(); + return this.incompleteOperation; } }, jobs, OperationType.CLEANUP); } From f4b283c0b8d499ade2901f42da2df63035dc0f09 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 14 Apr 2026 15:54:33 -0700 Subject: [PATCH 17/27] Accord module changes --- modules/accord | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/accord b/modules/accord index 93d78be37ef9..ca673853df05 160000 --- a/modules/accord +++ b/modules/accord @@ -1 +1 @@ -Subproject commit 93d78be37ef904118da2426716b5d77c0fb227db +Subproject commit ca673853df058b45a39dd004f282375d79d94f56 From 62ce86a6fed48bddae8ac25e30d49027be974e7d Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Thu, 16 Apr 2026 16:16:04 -0700 Subject: [PATCH 18/27] addressed comments --- .../db/compaction/CompactionManager.java | 110 +++++++------ .../service/accord/AccordService.java | 11 +- .../service/accord/IAccordService.java | 4 +- .../accord/AccordNodetoolCleanupTest.java | 152 +++++++++++------- 4 files changed, 167 insertions(+), 110 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index 26760188eed7..11ade6fac89b 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -115,6 +115,7 @@ import org.apache.cassandra.repair.NoSuchRepairSessionException; import org.apache.cassandra.schema.CompactionParams.TombstoneOption; import org.apache.cassandra.schema.Schema; +import org.apache.cassandra.schema.TableId; import org.apache.cassandra.schema.TableMetadata; import org.apache.cassandra.service.ActiveRepairService; import org.apache.cassandra.service.StorageService; @@ -135,6 +136,7 @@ import org.apache.cassandra.utils.concurrent.Promise; import org.apache.cassandra.utils.concurrent.Refs; +import static com.google.common.base.Preconditions.checkState; import static java.util.Collections.singleton; import static org.apache.cassandra.concurrent.ExecutorFactory.Global.executorFactory; import static org.apache.cassandra.concurrent.FutureTask.callable; @@ -645,7 +647,8 @@ private static interface OneSSTableOperation { Iterable filterSSTables(LifecycleTransaction transaction); void execute(LifecycleTransaction input) throws IOException; - default boolean incompleteOperation() { + default boolean incompleteOperation() + { return false; }; } @@ -802,24 +805,10 @@ public AllSSTableOpStatus performCleanup(final ColumnFamilyStore cfStore, int jo if (partitioner.getClass() == LocalPartitioner.class) localWrites = RangesAtEndpoint.of(Replica.fullReplica(local, new Range<>(partitioner.getMinimumToken(), partitioner.getMinimumToken()))); - Set> noLongerOwnedRangesInUseByAccord = new HashSet<>(); - if (AccordService.isSetup()) - { - if (!localWrites.onlyTransient().ranges().isEmpty()) - { - logger.error("Transient replication is not supported for Accord"); - return AllSSTableOpStatus.ABORTED; - } - - if (AccordService.instance().getInUseRanges().containsKey(cfStore.getTableId())) - { - Set> accordOwnedRanges = AccordService.instance().getInUseRanges().get(cfStore.getTableId()); - for (Range range : accordOwnedRanges) - noLongerOwnedRangesInUseByAccord.addAll(range.subtractAll(localWrites.ranges())); - } - } + InUseRanges inUseRanges = getInUseRanges(cfStore.getTableId(), localWrites); - final Set> allRanges = Stream.concat(localWrites.ranges().stream(), noLongerOwnedRangesInUseByAccord.stream()).collect(Collectors.toSet()); + final List> noLongerOwnedRangesInUseByAccord = inUseRanges.noLongerOwnedRangesInUseByAccord; + final List> allRanges = inUseRanges.allRanges; final Set> transientRanges = new HashSet<>(localWrites.onlyTransient().ranges()); final Set> fullRanges = new HashSet<>(localWrites.onlyFull().ranges()); @@ -839,6 +828,8 @@ public Iterable filterSSTables(LifecycleTransaction transaction) SSTableReader sstable = sstableIter.next(); boolean needsCleanupFull = needsCleanup(sstable, fullRanges); boolean needsCleanupTransient = !transientRanges.isEmpty() && sstable.isRepaired() && needsCleanup(sstable, transientRanges); + boolean needsCleanupAccord = needsCleanup(sstable, noLongerOwnedRangesInUseByAccord); + //If there are no ranges for which the table needs cleanup either due to lack of intersection or lack //of the table being repaired. totalSSTables++; @@ -855,6 +846,13 @@ public Iterable filterSSTables(LifecycleTransaction transaction) transaction.cancel(sstable); skippedSStables++; } + else if (!needsCleanupAccord) + { + sstableIter.remove(); + transaction.cancel(sstable); + skippedSStables++; + this.incompleteOperation = true; + } } logger.info("Skipping cleanup for {}/{} sstables for {}.{} since they are fully contained in owned ranges (full ranges: {}, transient ranges: {})", @@ -867,8 +865,7 @@ public Iterable filterSSTables(LifecycleTransaction transaction) public void execute(LifecycleTransaction txn) throws IOException { CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfStore, allRanges, transientRanges, txn.onlyOne().isRepaired(), FBUtilities.nowInSeconds()); - if (doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes, noLongerOwnedRangesInUseByAccord)) - this.incompleteOperation = true; + this.incompleteOperation |= (doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes, this.incompleteOperation, noLongerOwnedRangesInUseByAccord)); } @Override @@ -879,6 +876,39 @@ public boolean incompleteOperation() }, jobs, OperationType.CLEANUP); } + public static class InUseRanges + { + public List> noLongerOwnedRangesInUseByAccord; + public List> allRanges; + + public InUseRanges(List> noLongerOwnedRangesInUseByAccord, List> allRanges) + { + this.noLongerOwnedRangesInUseByAccord = noLongerOwnedRangesInUseByAccord; + this.allRanges = allRanges; + } + } + + private InUseRanges getInUseRanges(TableId tableId, RangesAtEndpoint localWrites) + { + List> noLongerOwnedRangesInUseByAccord = new ArrayList<>(); + TableMetadata metadata = Schema.instance.getTableMetadata(tableId); + + if (metadata != null && metadata.isAccordEnabled()) + { + checkState(localWrites.onlyTransient().ranges().isEmpty(), "Transient Replication is not supported with Accord"); + Map>> inUseRanges = AccordService.instance().getInUseRanges(); + if (inUseRanges.containsKey(tableId)) + { + Set> accordOwnedTableRanges = inUseRanges.get(tableId); + for (Range range : accordOwnedTableRanges) + noLongerOwnedRangesInUseByAccord.addAll(range.subtractAll(localWrites.ranges())); + } + } + + Collection> allRanges = Stream.concat(localWrites.ranges().stream(), noLongerOwnedRangesInUseByAccord.stream()).collect(Collectors.toSet()); + return new InUseRanges(Range.normalize(noLongerOwnedRangesInUseByAccord), Range.normalize(allRanges)); + } + public AllSSTableOpStatus performGarbageCollection(final ColumnFamilyStore cfStore, TombstoneOption tombstoneOption, int jobs) throws InterruptedException, ExecutionException { assert !cfStore.isIndex(); @@ -1432,25 +1462,11 @@ public void forceUserDefinedCleanup(String dataFiles) Keyspace keyspace = cfs.keyspace; final RangesAtEndpoint replicas = StorageService.instance.getLocalReplicas(keyspace.getName()); - Set> noLongerOwnedRangesInUseByAccord = new HashSet<>(); - if (AccordService.isSetup()) - { - if (!replicas.onlyTransient().ranges().isEmpty()) - { - logger.error("Transient replication is not supported for Accord"); - return; - } - - if (AccordService.instance().getInUseRanges().containsKey(cfs.getTableId())) - { - Set> accordOwnedRanges = AccordService.instance().getInUseRanges().get(cfs.getTableId()); - for (Range range : accordOwnedRanges) - noLongerOwnedRangesInUseByAccord.addAll(range.subtractAll(replicas.ranges())); - } - } + InUseRanges inUseRanges = getInUseRanges(cfs.getTableId(), replicas); - final Set> allRanges = Stream.concat(replicas.ranges().stream(), noLongerOwnedRangesInUseByAccord.stream()).collect(Collectors.toSet()); - final Set> transientRanges = new HashSet<>(replicas.onlyTransient().ranges()); + final List> noLongerOwnedRangesInUseByAccord = inUseRanges.noLongerOwnedRangesInUseByAccord; + final List> allRanges = inUseRanges.allRanges; + final Set> transientRanges = replicas.onlyTransient().ranges(); boolean hasIndexes = cfs.indexManager.hasIndexes(); SSTableReader sstable = lookupSSTable(cfs, entry.getValue()); @@ -1464,7 +1480,7 @@ public void forceUserDefinedCleanup(String dataFiles) CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfs, allRanges, transientRanges, sstable.isRepaired(), FBUtilities.nowInSeconds()); try (LifecycleTransaction txn = cfs.getTracker().tryModify(sstable, OperationType.CLEANUP)) { - doCleanupOne(cfs, txn, cleanupStrategy, allRanges, hasIndexes, noLongerOwnedRangesInUseByAccord); + doCleanupOne(cfs, txn, cleanupStrategy, allRanges, hasIndexes, false, noLongerOwnedRangesInUseByAccord); } catch (IOException e) { @@ -1665,14 +1681,15 @@ public static boolean needsCleanup(SSTableReader sstable, Collection> allRanges, - boolean hasIndexes, - Collection> noLongerOwnedRangesInUseByAccord) throws IOException + LifecycleTransaction txn, + CleanupStrategy cleanupStrategy, + Collection> allRanges, + boolean hasIndexes, + boolean alreadyIncomplete, + Collection> noLongerOwnedRangesInUseByAccord) throws IOException { assert !cfs.isIndex(); - boolean incompleteOperation = false; + boolean incompleteOperation = alreadyIncomplete; SSTableReader sstable = txn.onlyOne(); @@ -1723,7 +1740,8 @@ private boolean doCleanupOne(final ColumnFamilyStore cfs, if (notCleaned == null) continue; - if (Range.isInRanges(partition.partitionKey().getToken(), noLongerOwnedRangesInUseByAccord)) + if (!noLongerOwnedRangesInUseByAccord.isEmpty() && !incompleteOperation + && Range.isInRanges(partition.partitionKey().getToken(), noLongerOwnedRangesInUseByAccord)) incompleteOperation = true; if (writer.append(notCleaned) != null) diff --git a/src/java/org/apache/cassandra/service/accord/AccordService.java b/src/java/org/apache/cassandra/service/accord/AccordService.java index e0859c3398cb..71fe3947a843 100644 --- a/src/java/org/apache/cassandra/service/accord/AccordService.java +++ b/src/java/org/apache/cassandra/service/accord/AccordService.java @@ -1186,13 +1186,12 @@ public Map>> getInUseRanges() for (Ranges ranges : globalRanges) { - ranges.stream().forEach(r -> { + for (accord.primitives.Range r : ranges) + { TokenRange tokenRange = (TokenRange) r; - if (!tableToRangeMap.containsKey(tokenRange.table())) - tableToRangeMap.put(tokenRange.table(), new HashSet<>()); - - tableToRangeMap.get(tokenRange.table()).add(tokenRange.toKeyspaceRange()); - }); + tableToRangeMap.computeIfAbsent(tokenRange.table(), tableId -> new HashSet<>()) + .add(tokenRange.toKeyspaceRange()); + } } return tableToRangeMap; diff --git a/src/java/org/apache/cassandra/service/accord/IAccordService.java b/src/java/org/apache/cassandra/service/accord/IAccordService.java index 9349e9a63afe..43ebb9493347 100644 --- a/src/java/org/apache/cassandra/service/accord/IAccordService.java +++ b/src/java/org/apache/cassandra/service/accord/IAccordService.java @@ -19,8 +19,8 @@ package org.apache.cassandra.service.accord; import java.util.Collection; +import java.util.Collections; import java.util.EnumSet; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -362,7 +362,7 @@ public void awaitDone(TableId id, long epoch) @Override public Map>> getInUseRanges() { - return new HashMap<>(); + return Collections.emptyMap(); } @Override diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index 3a6eeedbe949..3cf4722a36de 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -18,40 +18,65 @@ package org.apache.cassandra.distributed.test.accord; +import java.io.IOException; +import java.util.List; + +import org.apache.cassandra.Util; import org.apache.cassandra.db.Keyspace; -import org.apache.cassandra.distributed.Cluster; +import org.apache.cassandra.dht.Murmur3Partitioner; import org.apache.cassandra.distributed.api.ConsistencyLevel; import org.apache.cassandra.distributed.api.Feature; import org.apache.cassandra.distributed.api.NodeToolResult; import org.apache.cassandra.distributed.api.SimpleQueryResult; -import org.apache.cassandra.distributed.test.TestBaseImpl; +import org.apache.cassandra.schema.Schema; +import org.apache.cassandra.schema.TableId; import org.apache.cassandra.service.StorageService; +import org.apache.cassandra.service.accord.AccordService; +import org.apache.cassandra.service.accord.api.TokenKey; import static com.google.common.collect.Iterables.getOnlyElement; +import static org.apache.cassandra.service.accord.AccordService.getBlocking; import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.fail; + +import org.junit.BeforeClass; import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import accord.api.RoutingKey; +import accord.primitives.Ranges; -public class AccordNodetoolCleanupTest extends TestBaseImpl +public class AccordNodetoolCleanupTest extends AccordTestBase { + private static final Logger logger = LoggerFactory.getLogger(AccordNodetoolCleanupTest.class); + + @Override + protected Logger logger() + { + return logger; + } + + @BeforeClass + public static void setupClass() throws IOException + { + AccordTestBase.setupCluster(builder -> builder + .withoutVNodes() + .appendConfig(config -> config + .set("accord.shard_durability_cycle", "20s") + .with(Feature.GOSSIP, Feature.NETWORK)), 2); + SHARED_CLUSTER.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';'); + SHARED_CLUSTER.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); + } + @Test public void accordNodetoolCleanupTest() throws Throwable { String tableName = "tbl0"; String qualifiedTableName = KEYSPACE + '.' + tableName; - try (Cluster cluster = init(builder().withNodes(2).withoutVNodes().withConfig((config) -> - config - .set("accord.shard_durability_target_splits", "1") - .set("accord.shard_durability_cycle", "20s") - .with(Feature.NETWORK, Feature.GOSSIP)).start())) - { - cluster.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE); - cluster.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); - cluster.schemaChange("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); - SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); cluster.get(1).flush(withKeyspace("%s")); @@ -70,36 +95,42 @@ public void accordNodetoolCleanupTest() throws Throwable }); // Wait until Accord retires range, so it no longer has ownership of token - try - { - Thread.sleep(20000); - } - catch (InterruptedException e) - { - fail(); - } + cluster.get(1).runOnInstance(() -> { + TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); + RoutingKey key = TokenKey.parse(tid, String.valueOf(token), Murmur3Partitioner.instance); + + Util.spinUntilTrue(() -> { + boolean doesNotContainsToken = true; + List inUseRanges = getBlocking(AccordService.instance().node().commandStores().getInUseRanges()); + for (Ranges ranges : inUseRanges) + { + if (ranges.intersects(key)) + doesNotContainsToken = false; + } + return doesNotContainsToken; + }); + }); cluster.get(1).nodetool("cleanup", KEYSPACE, tableName); assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - } + + // Ensure data is cleaned up + assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(originalToken); + }); + }); } @Test public void accordNodetoolCleanupPartialSSTableTest() throws Throwable { - String tableName = "tbl0"; + String tableName = "tbl1"; String qualifiedTableName = KEYSPACE + '.' + tableName; - try (Cluster cluster = init(builder().withNodes(2).withoutVNodes().withConfig((config) -> - config - .set("accord.shard_durability_target_splits", "1") - .set("accord.shard_durability_cycle", "20s") - .with(Feature.NETWORK, Feature.GOSSIP)).start())) - { - cluster.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE); - cluster.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); - cluster.schemaChange("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 2, 2); @@ -127,23 +158,27 @@ public void accordNodetoolCleanupPartialSSTableTest() throws Throwable assertTrue(result.getStdout().contains("Partially cleaned up SSTables for ranges that are no longer owned in keyspace " + KEYSPACE)); + assertEquals(2, result.getRc()); assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - } + + // Ensure data is still there + assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 2 LIMIT 1").length); + + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(originalToken); + }); + }); + } @Test public void accordNodetoolCleanupRangeInUseTest() throws Throwable { - String tableName = "tbl0"; + String tableName = "tbl2"; String qualifiedTableName = KEYSPACE + '.' + tableName; - try (Cluster cluster = init(builder().withNodes(2).withoutVNodes().withConfig((config) -> - config - .with(Feature.NETWORK, Feature.GOSSIP)).start())) - { - cluster.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE); - cluster.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); - cluster.schemaChange("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'"); + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); @@ -162,27 +197,25 @@ public void accordNodetoolCleanupRangeInUseTest() throws Throwable cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); - // Cluster 1 no longer owns token, however Accord still needs it so it is no cleaned up + // Cluster 1 no longer owns token, however Accord still needs it so it is not cleaned up assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - } + + // Ensure data is still there + assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(originalToken); + }); + }); } @Test public void nodetoolCleanupForNonAccordTableTest() throws Throwable { - String tableName = "tbl0"; + String tableName = "tbl3"; String qualifiedTableName = KEYSPACE + '.' + tableName; - try (Cluster cluster = init(builder().withNodes(2).withoutVNodes().withConfig((config) -> - config - .set("accord.shard_durability_target_splits", "1") - .set("accord.shard_durability_cycle", "20s") - .with(Feature.NETWORK, Feature.GOSSIP)).start())) - { - - cluster.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE); - cluster.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); - cluster.schemaChange("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int)"); + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int)", cluster -> { cluster.coordinator(1).execute("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)", ConsistencyLevel.ALL, 1, 2); SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); @@ -204,7 +237,14 @@ public void nodetoolCleanupForNonAccordTableTest() throws Throwable cluster.get(1).nodetool("cleanup", KEYSPACE, tableName); assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - } + + // Ensure data is cleaned up + assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(originalToken); + }); + }); } } From dd79e009268370f79669ed2a02720044cdd9b9eb Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Thu, 14 May 2026 21:29:31 -0700 Subject: [PATCH 19/27] fixes --- .../db/compaction/CompactionManager.java | 24 ++++--- .../org/apache/cassandra/tools/NodeProbe.java | 4 +- .../accord/AccordNodetoolCleanupTest.java | 72 ++++++++++--------- 3 files changed, 55 insertions(+), 45 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index 11ade6fac89b..83bca17911f6 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -584,6 +584,11 @@ private AllSSTableOpStatus parallelAllSSTableOperation(final ColumnFamilyStore c Iterable sstables = Lists.newArrayList(operation.filterSSTables(compacting)); if (Iterables.isEmpty(sstables)) { + if (operation.incompleteOperation()) + { + logger.info("Operation incomplete for {}.{}", keyspace, table); + return AllSSTableOpStatus.INCOMPLETE; + } logger.info("No sstables to {} for {}.{}", operationName, keyspace, table); return AllSSTableOpStatus.SUCCESSFUL; } @@ -814,7 +819,7 @@ public AllSSTableOpStatus performCleanup(final ColumnFamilyStore cfStore, int jo return parallelAllSSTableOperation(cfStore, new OneSSTableOperation() { - boolean incompleteOperation = false; + volatile boolean incompleteOperation = false; @Override public Iterable filterSSTables(LifecycleTransaction transaction) @@ -822,7 +827,8 @@ public Iterable filterSSTables(LifecycleTransaction transaction) List sortedSSTables = Lists.newArrayList(transaction.originals()); Iterator sstableIter = sortedSSTables.iterator(); int totalSSTables = 0; - int skippedSStables = 0; + int containedInRangeSkippedSStables = 0; + int inUseByAccordSkippedSStables = 0; while (sstableIter.hasNext()) { SSTableReader sstable = sstableIter.next(); @@ -844,19 +850,20 @@ public Iterable filterSSTables(LifecycleTransaction transaction) sstable.isRepaired()); sstableIter.remove(); transaction.cancel(sstable); - skippedSStables++; + containedInRangeSkippedSStables++; } else if (!needsCleanupAccord) { sstableIter.remove(); transaction.cancel(sstable); - skippedSStables++; + inUseByAccordSkippedSStables++; this.incompleteOperation = true; } } - logger.info("Skipping cleanup for {}/{} sstables for {}.{} since they are fully contained in owned ranges (full ranges: {}, transient ranges: {})", - skippedSStables, totalSSTables, cfStore.getKeyspaceName(), cfStore.getTableName(), fullRanges, transientRanges); + logger.info("Skipping cleanup for {}/{} sstables since they are fully contained in owned ranges and skipping cleanup for {}/{} since they are in use by Accord " + + "for {}.{} (full ranges: {}, transient ranges: {})", containedInRangeSkippedSStables, totalSSTables, inUseByAccordSkippedSStables, totalSSTables, + cfStore.getKeyspaceName(), cfStore.getTableName(), fullRanges, transientRanges); sortedSSTables.sort(SSTableReader.sizeComparator); return sortedSSTables; } @@ -865,7 +872,8 @@ else if (!needsCleanupAccord) public void execute(LifecycleTransaction txn) throws IOException { CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfStore, allRanges, transientRanges, txn.onlyOne().isRepaired(), FBUtilities.nowInSeconds()); - this.incompleteOperation |= (doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes, this.incompleteOperation, noLongerOwnedRangesInUseByAccord)); + if (doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes, this.incompleteOperation, noLongerOwnedRangesInUseByAccord)) + this.incompleteOperation = true; } @Override @@ -893,7 +901,7 @@ private InUseRanges getInUseRanges(TableId tableId, RangesAtEndpoint localWrites List> noLongerOwnedRangesInUseByAccord = new ArrayList<>(); TableMetadata metadata = Schema.instance.getTableMetadata(tableId); - if (metadata != null && metadata.isAccordEnabled()) + if (metadata != null && metadata.requiresAccordSupport()) { checkState(localWrites.onlyTransient().ranges().isEmpty(), "Transient Replication is not supported with Accord"); Map>> inUseRanges = AccordService.instance().getInUseRanges(); diff --git a/src/java/org/apache/cassandra/tools/NodeProbe.java b/src/java/org/apache/cassandra/tools/NodeProbe.java index ae5599865a55..93c0fdb6cac1 100644 --- a/src/java/org/apache/cassandra/tools/NodeProbe.java +++ b/src/java/org/apache/cassandra/tools/NodeProbe.java @@ -481,8 +481,8 @@ private void perform(PrintStream out, String ks, Job job, String jobName) throws out.printf("Failed marking some sstables compacting in keyspace %s, check server logs for more information.\n", ks); throw new RuntimeException(String.format("Failed marking some sstables compacting in keyspace %s, check server logs for more information.\n", ks)); case 3: - out.printf("Partially cleaned up SSTables for ranges that are no longer owned in keyspace %s, check server logs for more information.\n", ks); - throw new RuntimeException(String.format("Partially cleaned up SSTables for ranges that are no longer owned in keyspace %s, check server logs for more information.\n", ks)); + out.printf("Some SSTables in keyspace %s are still being used by Accord and were not cleaned up, check server logs for more information.\n", ks); + throw new RuntimeException(String.format("Some SSTables in keyspace %s are still being used by Accord and were not cleaned up, check server logs for more information.\n", ks)); } } diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index 3cf4722a36de..f35d14d41d09 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -39,6 +39,8 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertEquals; +import org.junit.After; +import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; import org.slf4j.Logger; @@ -51,6 +53,8 @@ public class AccordNodetoolCleanupTest extends AccordTestBase { private static final Logger logger = LoggerFactory.getLogger(AccordNodetoolCleanupTest.class); + protected String originalToken; + @Override protected Logger logger() { @@ -63,12 +67,27 @@ public static void setupClass() throws IOException AccordTestBase.setupCluster(builder -> builder .withoutVNodes() .appendConfig(config -> config - .set("accord.shard_durability_cycle", "20s") + .set("accord.shard_durability_cycle", "1s") .with(Feature.GOSSIP, Feature.NETWORK)), 2); SHARED_CLUSTER.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';'); SHARED_CLUSTER.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); } + @Before + public void getOriginalToken() + { + originalToken = SHARED_CLUSTER.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); + } + + @After + public void reset() + { + String token = originalToken; + SHARED_CLUSTER.get(1).runOnInstance(() -> { + StorageService.instance.move(token); + }); + } + @Test public void accordNodetoolCleanupTest() throws Throwable { @@ -81,8 +100,6 @@ public void accordNodetoolCleanupTest() throws Throwable cluster.get(1).flush(withKeyspace("%s")); - String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); - long token = (Long) result.toObjectArrays()[0][0]; assertTrue(token < Long.parseLong(originalToken)); @@ -91,6 +108,7 @@ public void accordNodetoolCleanupTest() throws Throwable // Cluster 1 no longer owns token cluster.get(1).runOnInstance(() -> { + AccordService.instance().node().durability().shards().start(); StorageService.instance.move(Long.toString(token - 1000)); }); @@ -111,16 +129,13 @@ public void accordNodetoolCleanupTest() throws Throwable }); }); - cluster.get(1).nodetool("cleanup", KEYSPACE, tableName); + NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); + assertEquals(0, nodetoolResult.getRc()); assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); // Ensure data is cleaned up assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); - - cluster.get(1).runOnInstance(() -> { - StorageService.instance.move(originalToken); - }); }); } @@ -139,8 +154,6 @@ public void accordNodetoolCleanupPartialSSTableTest() throws Throwable cluster.get(1).flush(withKeyspace("%s")); - String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); - long token1 = (Long) result1.toObjectArrays()[0][0]; long token2 = (Long) result2.toObjectArrays()[0][0]; @@ -150,24 +163,19 @@ public void accordNodetoolCleanupPartialSSTableTest() throws Throwable // Cluster 1 now only owns token2, but Accord still requires token1 cluster.get(1).runOnInstance(() -> { - Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).disableAutoCompaction(); + AccordService.instance().node().durability().shards().stop(); StorageService.instance.move(Long.toString(token1 - 1000)); }); - NodeToolResult result = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); - - assertTrue(result.getStdout().contains("Partially cleaned up SSTables for ranges that are no longer owned in keyspace " + KEYSPACE)); + NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); - assertEquals(2, result.getRc()); + assertTrue(nodetoolResult.getStdout().contains("Some SSTables in keyspace " + KEYSPACE + " are still being used by Accord and were not cleaned up, check server logs for more information.")); + assertEquals(2, nodetoolResult.getRc()); assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); // Ensure data is still there assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 2 LIMIT 1").length); - - cluster.get(1).runOnInstance(() -> { - StorageService.instance.move(originalToken); - }); }); } @@ -185,27 +193,25 @@ public void accordNodetoolCleanupRangeInUseTest() throws Throwable cluster.get(1).flush(withKeyspace("%s")); - String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); - long token = (Long) result.toObjectArrays()[0][0]; assertTrue(token < Long.parseLong(originalToken)); assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - cluster.get(1).runOnInstance(() -> StorageService.instance.move(Long.toString(token - 1000))); + cluster.get(1).runOnInstance(() -> { + AccordService.instance().node().durability().shards().stop(); + StorageService.instance.move(Long.toString(token - 1000)); + }); - cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); + NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); - // Cluster 1 no longer owns token, however Accord still needs it so it is not cleaned up + assertTrue(nodetoolResult.getStdout().contains("Some SSTables in keyspace " + KEYSPACE + " are still being used by Accord and were not cleaned up, check server logs for more information.")); + assertEquals(2, nodetoolResult.getRc()); assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); // Ensure data is still there assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); - - cluster.get(1).runOnInstance(() -> { - StorageService.instance.move(originalToken); - }); }); } @@ -222,8 +228,6 @@ public void nodetoolCleanupForNonAccordTableTest() throws Throwable cluster.get(1).flush(withKeyspace("%s")); - String originalToken = cluster.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); - long token = (Long) result.toObjectArrays()[0][0]; assertTrue(token < Long.parseLong(originalToken)); @@ -231,19 +235,17 @@ public void nodetoolCleanupForNonAccordTableTest() throws Throwable assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); cluster.get(1).runOnInstance(() -> { + AccordService.instance().node().durability().shards().stop(); StorageService.instance.move(Long.toString(token - 1000)); }); - cluster.get(1).nodetool("cleanup", KEYSPACE, tableName); + NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); + assertEquals(0, nodetoolResult.getRc()); assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); // Ensure data is cleaned up assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); - - cluster.get(1).runOnInstance(() -> { - StorageService.instance.move(originalToken); - }); }); } } From 528c4325f638aabb290287a8d22d7e59380c4db6 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Thu, 14 May 2026 23:31:15 -0700 Subject: [PATCH 20/27] fix test flakiness --- .../distributed/test/accord/AccordNodetoolCleanupTest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index f35d14d41d09..a52d727defa2 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -36,8 +36,9 @@ import static com.google.common.collect.Iterables.getOnlyElement; import static org.apache.cassandra.service.accord.AccordService.getBlocking; -import static org.junit.Assert.assertTrue; + import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import org.junit.After; import org.junit.Before; @@ -67,7 +68,7 @@ public static void setupClass() throws IOException AccordTestBase.setupCluster(builder -> builder .withoutVNodes() .appendConfig(config -> config - .set("accord.shard_durability_cycle", "1s") + .set("accord.shard_durability_cycle", "20s") .with(Feature.GOSSIP, Feature.NETWORK)), 2); SHARED_CLUSTER.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';'); SHARED_CLUSTER.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); @@ -126,7 +127,7 @@ public void accordNodetoolCleanupTest() throws Throwable doesNotContainsToken = false; } return doesNotContainsToken; - }); + }, 30); }); NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); From abc708d6dfdb457a1899d088aa5390245e26def7 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Mon, 18 May 2026 11:28:00 -0700 Subject: [PATCH 21/27] fix import order --- .../accord/AccordNodetoolCleanupTest.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index a52d727defa2..00cc17b16977 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -21,6 +21,16 @@ import java.io.IOException; import java.util.List; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import accord.api.RoutingKey; +import accord.primitives.Ranges; + import org.apache.cassandra.Util; import org.apache.cassandra.db.Keyspace; import org.apache.cassandra.dht.Murmur3Partitioner; @@ -36,20 +46,9 @@ import static com.google.common.collect.Iterables.getOnlyElement; import static org.apache.cassandra.service.accord.AccordService.getBlocking; - import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import org.junit.After; -import org.junit.Before; -import org.junit.BeforeClass; -import org.junit.Test; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import accord.api.RoutingKey; -import accord.primitives.Ranges; - public class AccordNodetoolCleanupTest extends AccordTestBase { private static final Logger logger = LoggerFactory.getLogger(AccordNodetoolCleanupTest.class); From fdedf387aa7c61da7a8ad3c45c1b02f97493ca3c Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Mon, 18 May 2026 12:13:23 -0700 Subject: [PATCH 22/27] mark unsafe to read for retired ranges we are cleaning up --- modules/accord | 2 +- .../apache/cassandra/db/compaction/CompactionManager.java | 2 +- .../apache/cassandra/service/accord/AccordService.java | 4 ++-- .../apache/cassandra/service/accord/IAccordService.java | 8 ++++---- .../test/accord/AccordNodetoolCleanupTest.java | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/modules/accord b/modules/accord index ca673853df05..28d5bca978c7 160000 --- a/modules/accord +++ b/modules/accord @@ -1 +1 @@ -Subproject commit ca673853df058b45a39dd004f282375d79d94f56 +Subproject commit 28d5bca978c75b9c8f4e5ed745e7376395b16df5 diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index 83bca17911f6..19496c1dcf90 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -904,7 +904,7 @@ private InUseRanges getInUseRanges(TableId tableId, RangesAtEndpoint localWrites if (metadata != null && metadata.requiresAccordSupport()) { checkState(localWrites.onlyTransient().ranges().isEmpty(), "Transient Replication is not supported with Accord"); - Map>> inUseRanges = AccordService.instance().getInUseRanges(); + Map>> inUseRanges = AccordService.instance().getInUseRangesAndMarkRetiredRangesUnsafeToRead(); if (inUseRanges.containsKey(tableId)) { Set> accordOwnedTableRanges = inUseRanges.get(tableId); diff --git a/src/java/org/apache/cassandra/service/accord/AccordService.java b/src/java/org/apache/cassandra/service/accord/AccordService.java index 71fe3947a843..1742f5e6668f 100644 --- a/src/java/org/apache/cassandra/service/accord/AccordService.java +++ b/src/java/org/apache/cassandra/service/accord/AccordService.java @@ -1179,10 +1179,10 @@ public void awaitDone(TableId id, long epoch) } @Override - public Map>> getInUseRanges() + public Map>> getInUseRangesAndMarkRetiredRangesUnsafeToRead() { Map>> tableToRangeMap = new HashMap<>(); - List globalRanges = getBlocking(node.commandStores().getInUseRanges()); + List globalRanges = getBlocking(node.commandStores().getInUseRangesAndMarkRetiredRangesUnsafeToRead()); for (Ranges ranges : globalRanges) { diff --git a/src/java/org/apache/cassandra/service/accord/IAccordService.java b/src/java/org/apache/cassandra/service/accord/IAccordService.java index 43ebb9493347..2aaf8e07fc7d 100644 --- a/src/java/org/apache/cassandra/service/accord/IAccordService.java +++ b/src/java/org/apache/cassandra/service/accord/IAccordService.java @@ -188,7 +188,7 @@ public AccordCompactionInfos(DurableBefore durableBefore, long minEpoch, AccordC void awaitDone(TableId id, long epoch); - Map>> getInUseRanges(); + Map>> getInUseRangesAndMarkRetiredRangesUnsafeToRead(); AccordEndpointMapper endpointMapper(); @@ -360,7 +360,7 @@ public void awaitDone(TableId id, long epoch) } @Override - public Map>> getInUseRanges() + public Map>> getInUseRangesAndMarkRetiredRangesUnsafeToRead() { return Collections.emptyMap(); } @@ -576,9 +576,9 @@ public void awaitDone(TableId id, long epoch) } @Override - public Map>> getInUseRanges() + public Map>> getInUseRangesAndMarkRetiredRangesUnsafeToRead() { - return delegate.getInUseRanges(); + return delegate.getInUseRangesAndMarkRetiredRangesUnsafeToRead(); } @Override diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index 00cc17b16977..d0c7c1c73edc 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -119,7 +119,7 @@ public void accordNodetoolCleanupTest() throws Throwable Util.spinUntilTrue(() -> { boolean doesNotContainsToken = true; - List inUseRanges = getBlocking(AccordService.instance().node().commandStores().getInUseRanges()); + List inUseRanges = getBlocking(AccordService.instance().node().commandStores().getInUseRangesAndMarkRetiredRangesUnsafeToRead()); for (Ranges ranges : inUseRanges) { if (ranges.intersects(key)) From fe539d9b2b318f41184b6c9016db6907743f14e1 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Mon, 1 Jun 2026 13:17:29 -0400 Subject: [PATCH 23/27] fix comment --- .../db/compaction/CompactionManager.java | 6 ++-- .../accord/AccordNodetoolCleanupTest.java | 28 +++++++++++++++---- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java index 19496c1dcf90..11bb019709e8 100644 --- a/src/java/org/apache/cassandra/db/compaction/CompactionManager.java +++ b/src/java/org/apache/cassandra/db/compaction/CompactionManager.java @@ -655,7 +655,7 @@ private static interface OneSSTableOperation default boolean incompleteOperation() { return false; - }; + } } public enum AllSSTableOpStatus @@ -886,8 +886,8 @@ public boolean incompleteOperation() public static class InUseRanges { - public List> noLongerOwnedRangesInUseByAccord; - public List> allRanges; + public final List> noLongerOwnedRangesInUseByAccord; + public final List> allRanges; public InUseRanges(List> noLongerOwnedRangesInUseByAccord, List> allRanges) { diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index d0c7c1c73edc..0e1b1bfdb9ab 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -156,15 +156,33 @@ public void accordNodetoolCleanupPartialSSTableTest() throws Throwable long token1 = (Long) result1.toObjectArrays()[0][0]; long token2 = (Long) result2.toObjectArrays()[0][0]; + long newToken = token1 - 1000; - assertTrue((token2 < (token1 - 1000)) && token1 < Long.parseLong(originalToken)); + assertTrue((token2 < newToken) && (token1 < Long.parseLong(originalToken))); assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); // Cluster 1 now only owns token2, but Accord still requires token1 cluster.get(1).runOnInstance(() -> { - AccordService.instance().node().durability().shards().stop(); - StorageService.instance.move(Long.toString(token1 - 1000)); + AccordService.instance().node().durability().shards().start(); + StorageService.instance.move(Long.toString(newToken)); + }); + + // Wait until Accord retires range, so it no longer has ownership of token1 + cluster.get(1).runOnInstance(() -> { + TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); + RoutingKey key = TokenKey.parse(tid, String.valueOf(token1), Murmur3Partitioner.instance); + + Util.spinUntilTrue(() -> { + boolean doesNotContainsToken = true; + List inUseRanges = getBlocking(AccordService.instance().node().commandStores().getInUseRangesAndMarkRetiredRangesUnsafeToRead()); + for (Ranges ranges : inUseRanges) + { + if (ranges.intersects(key)) + doesNotContainsToken = false; + } + return doesNotContainsToken; + }, 30); }); NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); @@ -173,9 +191,9 @@ public void accordNodetoolCleanupPartialSSTableTest() throws Throwable assertEquals(2, nodetoolResult.getRc()); assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - // Ensure data is still there + // Ensure data is correct assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); - assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 2 LIMIT 1").length); + assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 2 LIMIT 1").length); }); } From 42c6b468877d78edaecc0d1a17760ce16b7a927b Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Mon, 1 Jun 2026 13:50:24 -0400 Subject: [PATCH 24/27] fix test --- .../distributed/test/accord/AccordNodetoolCleanupTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java index 0e1b1bfdb9ab..fe5e9813f68f 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java @@ -153,6 +153,7 @@ public void accordNodetoolCleanupPartialSSTableTest() throws Throwable SimpleQueryResult result2 = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); cluster.get(1).flush(withKeyspace("%s")); + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); long token1 = (Long) result1.toObjectArrays()[0][0]; long token2 = (Long) result2.toObjectArrays()[0][0]; @@ -187,8 +188,7 @@ public void accordNodetoolCleanupPartialSSTableTest() throws Throwable NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); - assertTrue(nodetoolResult.getStdout().contains("Some SSTables in keyspace " + KEYSPACE + " are still being used by Accord and were not cleaned up, check server logs for more information.")); - assertEquals(2, nodetoolResult.getRc()); + assertEquals(0, nodetoolResult.getRc()); assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); // Ensure data is correct From 3c4a9f4b1a610d8f624b86720915678f31efd7ee Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Mon, 1 Jun 2026 15:45:24 -0400 Subject: [PATCH 25/27] include tests for force user defined cleanup --- .../test/accord/AccordCleanupTest.java | 374 ++++++++++++++++++ .../accord/AccordNodetoolCleanupTest.java | 270 ------------- 2 files changed, 374 insertions(+), 270 deletions(-) create mode 100644 test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java delete mode 100644 test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java new file mode 100644 index 000000000000..a1175d38fe75 --- /dev/null +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java @@ -0,0 +1,374 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.distributed.test.accord; + +import java.io.IOException; +import java.util.List; + +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import accord.api.RoutingKey; +import accord.primitives.Ranges; + +import org.apache.cassandra.Util; +import org.apache.cassandra.db.Keyspace; +import org.apache.cassandra.db.compaction.CompactionManager; +import org.apache.cassandra.dht.Murmur3Partitioner; +import org.apache.cassandra.distributed.Cluster; +import org.apache.cassandra.distributed.api.ConsistencyLevel; +import org.apache.cassandra.distributed.api.Feature; +import org.apache.cassandra.distributed.api.NodeToolResult; +import org.apache.cassandra.distributed.api.SimpleQueryResult; +import org.apache.cassandra.schema.Schema; +import org.apache.cassandra.schema.TableId; +import org.apache.cassandra.service.StorageService; +import org.apache.cassandra.service.accord.AccordService; +import org.apache.cassandra.service.accord.api.TokenKey; + +import static com.google.common.collect.Iterables.getOnlyElement; +import static org.apache.cassandra.service.accord.AccordService.getBlocking; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class AccordCleanupTest extends AccordTestBase +{ + private static final Logger logger = LoggerFactory.getLogger(AccordCleanupTest.class); + + protected String originalToken; + + @Override + protected Logger logger() + { + return logger; + } + + @BeforeClass + public static void setupClass() throws IOException + { + AccordTestBase.setupCluster(builder -> builder + .withoutVNodes() + .appendConfig(config -> config + .set("accord.shard_durability_cycle", "25s") + .with(Feature.GOSSIP, Feature.NETWORK)), 2); + SHARED_CLUSTER.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';'); + SHARED_CLUSTER.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); + } + + @Before + public void getOriginalToken() + { + originalToken = SHARED_CLUSTER.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); + } + + @After + public void reset() + { + String token = originalToken; + SHARED_CLUSTER.get(1).runOnInstance(() -> { + StorageService.instance.move(token); + }); + } + + @Test + public void accordNodetoolCleanupTest() throws Throwable + { + String tableName = "tbl0"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { + // Loses ownership of k = 1 and Accord no longer needs it + setupForCleanupTest(cluster, tableName, qualifiedTableName); + + NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); + + assertEquals(0, nodetoolResult.getRc()); + assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + // Ensure data is cleaned up + assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + }); + } + + @Test + public void accordUserDefinedCleanupTest() throws Throwable + { + String tableName = "tbl1"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { + // Loses ownership of k = 1 and Accord no longer needs it + setupForCleanupTest(cluster, tableName, qualifiedTableName); + + cluster.get(1).runOnInstance(() -> { + String dataFile = getOnlyElement(Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables()).getFilename(); + CompactionManager.instance.forceUserDefinedCleanup(dataFile); + }); + + // Ensure data is cleaned up + assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + }); + } + + @Test + public void accordNodetoolCleanupPartialSSTableTest() throws Throwable + { + String tableName = "tbl2"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { + setupForCleanupPartialSSTableTest(cluster, tableName, qualifiedTableName); + + NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); + + assertEquals(0, nodetoolResult.getRc()); + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + // Ensure data is correct + assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 2 LIMIT 1").length); + }); + } + + @Test + public void accordUserDefinedCleanupPartialSSTableTest() throws Throwable + { + String tableName = "tbl3"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { + setupForCleanupPartialSSTableTest(cluster, tableName, qualifiedTableName); + + cluster.get(1).runOnInstance(() -> { + String dataFile = getOnlyElement(Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables()).getFilename(); + CompactionManager.instance.forceUserDefinedCleanup(dataFile); + }); + + // Ensure data is correct + assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 2 LIMIT 1").length); + }); + } + + @Test + public void accordNodetoolCleanupRangeInUseTest() throws Throwable + { + String tableName = "tbl4"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { + setupForCleanupRangeInUseTest(cluster, tableName, qualifiedTableName); + + NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); + + assertTrue(nodetoolResult.getStdout().contains("Some SSTables in keyspace " + KEYSPACE + " are still being used by Accord and were not cleaned up, check server logs for more information.")); + assertEquals(2, nodetoolResult.getRc()); + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + // Ensure data is still there + assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + }); + } + + @Test + public void accordUserDefinedCleanupRangeInUseTest() throws Throwable + { + String tableName = "tbl5"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { + setupForCleanupRangeInUseTest(cluster, tableName, qualifiedTableName); + + cluster.get(1).runOnInstance(() -> { + String dataFile = getOnlyElement(Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables()).getFilename(); + CompactionManager.instance.forceUserDefinedCleanup(dataFile); + }); + + // Ensure data is still there + assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + }); + } + + @Test + public void nodetoolCleanupForNonAccordTableTest() throws Throwable + { + String tableName = "tbl6"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int)", cluster -> { + setupForCleanupForNonAccordTableTest(cluster, tableName, qualifiedTableName); + + NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); + + assertEquals(0, nodetoolResult.getRc()); + assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + // Ensure data is cleaned up + assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + }); + } + + @Test + public void userDefinedCleanupForNonAccordTableTest() throws Throwable + { + String tableName = "tbl7"; + String qualifiedTableName = KEYSPACE + '.' + tableName; + + test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int)", cluster -> { + setupForCleanupForNonAccordTableTest(cluster, tableName, qualifiedTableName); + + cluster.get(1).runOnInstance(() -> { + String dataFile = getOnlyElement(Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables()).getFilename(); + CompactionManager.instance.forceUserDefinedCleanup(dataFile); + }); + + // Ensure data is cleaned up + assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); + }); + } + + private void setupForCleanupTest(Cluster cluster, String tableName, String qualifiedTableName) + { + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + cluster.get(1).flush(withKeyspace("%s")); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + long token = (Long) result.toObjectArrays()[0][0]; + + assertTrue(token < Long.parseLong(originalToken)); + + // Cluster 1 no longer owns token + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(Long.toString(token - 1000)); + AccordService.instance().node().durability().shards().start(); + }); + + // Wait until Accord retires range, so it no longer has ownership of token + cluster.get(1).runOnInstance(() -> { + TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); + RoutingKey key = TokenKey.parse(tid, String.valueOf(token), Murmur3Partitioner.instance); + + Util.spinUntilTrue(() -> { + boolean doesNotContainsToken = true; + List inUseRanges = getBlocking(AccordService.instance().node().commandStores().getInUseRangesAndMarkRetiredRangesUnsafeToRead()); + for (Ranges ranges : inUseRanges) + { + if (ranges.intersects(key)) + doesNotContainsToken = false; + } + return doesNotContainsToken; + }, 35); + }); + } + + private void setupForCleanupPartialSSTableTest(Cluster cluster, String tableName, String qualifiedTableName) + { + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 2, 2); + + SimpleQueryResult result1 = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 2 LIMIT 1", ConsistencyLevel.SERIAL); + SimpleQueryResult result2 = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + cluster.get(1).flush(withKeyspace("%s")); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + long token1 = (Long) result1.toObjectArrays()[0][0]; + long token2 = (Long) result2.toObjectArrays()[0][0]; + long newToken = token1 - 1000; + + assertTrue((token2 < newToken) && (token1 < Long.parseLong(originalToken))); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + // Cluster 1 now only owns token2, but Accord still requires token1 + cluster.get(1).runOnInstance(() -> { + StorageService.instance.move(Long.toString(newToken)); + AccordService.instance().node().durability().shards().start(); + }); + + // Wait until Accord retires range, so it no longer has ownership of token1 + cluster.get(1).runOnInstance(() -> { + TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); + RoutingKey key = TokenKey.parse(tid, String.valueOf(token1), Murmur3Partitioner.instance); + + Util.spinUntilTrue(() -> { + boolean doesNotContainsToken = true; + List inUseRanges = getBlocking(AccordService.instance().node().commandStores().getInUseRangesAndMarkRetiredRangesUnsafeToRead()); + for (Ranges ranges : inUseRanges) + { + if (ranges.intersects(key)) + doesNotContainsToken = false; + } + return doesNotContainsToken; + }, 35); + }); + } + + private void setupForCleanupRangeInUseTest(Cluster cluster, String tableName, String qualifiedTableName) + { + cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); + + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + cluster.get(1).flush(withKeyspace("%s")); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + long token = (Long) result.toObjectArrays()[0][0]; + + assertTrue(token < Long.parseLong(originalToken)); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + cluster.get(1).runOnInstance(() -> { + AccordService.instance().node().durability().shards().stop(); + StorageService.instance.move(Long.toString(token - 1000)); + }); + } + + private void setupForCleanupForNonAccordTableTest(Cluster cluster, String tableName, String qualifiedTableName) + { + cluster.coordinator(1).execute("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)", ConsistencyLevel.ALL, 1, 2); + + SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); + + cluster.get(1).flush(withKeyspace("%s")); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + long token = (Long) result.toObjectArrays()[0][0]; + + assertTrue(token < Long.parseLong(originalToken)); + + assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); + + cluster.get(1).runOnInstance(() -> { + AccordService.instance().node().durability().shards().stop(); + StorageService.instance.move(Long.toString(token - 1000)); + }); + } +} diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java deleted file mode 100644 index fe5e9813f68f..000000000000 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java +++ /dev/null @@ -1,270 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.cassandra.distributed.test.accord; - -import java.io.IOException; -import java.util.List; - -import org.junit.After; -import org.junit.Before; -import org.junit.BeforeClass; -import org.junit.Test; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import accord.api.RoutingKey; -import accord.primitives.Ranges; - -import org.apache.cassandra.Util; -import org.apache.cassandra.db.Keyspace; -import org.apache.cassandra.dht.Murmur3Partitioner; -import org.apache.cassandra.distributed.api.ConsistencyLevel; -import org.apache.cassandra.distributed.api.Feature; -import org.apache.cassandra.distributed.api.NodeToolResult; -import org.apache.cassandra.distributed.api.SimpleQueryResult; -import org.apache.cassandra.schema.Schema; -import org.apache.cassandra.schema.TableId; -import org.apache.cassandra.service.StorageService; -import org.apache.cassandra.service.accord.AccordService; -import org.apache.cassandra.service.accord.api.TokenKey; - -import static com.google.common.collect.Iterables.getOnlyElement; -import static org.apache.cassandra.service.accord.AccordService.getBlocking; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; - -public class AccordNodetoolCleanupTest extends AccordTestBase -{ - private static final Logger logger = LoggerFactory.getLogger(AccordNodetoolCleanupTest.class); - - protected String originalToken; - - @Override - protected Logger logger() - { - return logger; - } - - @BeforeClass - public static void setupClass() throws IOException - { - AccordTestBase.setupCluster(builder -> builder - .withoutVNodes() - .appendConfig(config -> config - .set("accord.shard_durability_cycle", "20s") - .with(Feature.GOSSIP, Feature.NETWORK)), 2); - SHARED_CLUSTER.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';'); - SHARED_CLUSTER.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); - } - - @Before - public void getOriginalToken() - { - originalToken = SHARED_CLUSTER.get(1).callOnInstance(() -> getOnlyElement(StorageService.instance.getTokens())); - } - - @After - public void reset() - { - String token = originalToken; - SHARED_CLUSTER.get(1).runOnInstance(() -> { - StorageService.instance.move(token); - }); - } - - @Test - public void accordNodetoolCleanupTest() throws Throwable - { - String tableName = "tbl0"; - String qualifiedTableName = KEYSPACE + '.' + tableName; - - test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { - cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); - SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); - - cluster.get(1).flush(withKeyspace("%s")); - - long token = (Long) result.toObjectArrays()[0][0]; - - assertTrue(token < Long.parseLong(originalToken)); - - assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - // Cluster 1 no longer owns token - cluster.get(1).runOnInstance(() -> { - AccordService.instance().node().durability().shards().start(); - StorageService.instance.move(Long.toString(token - 1000)); - }); - - // Wait until Accord retires range, so it no longer has ownership of token - cluster.get(1).runOnInstance(() -> { - TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); - RoutingKey key = TokenKey.parse(tid, String.valueOf(token), Murmur3Partitioner.instance); - - Util.spinUntilTrue(() -> { - boolean doesNotContainsToken = true; - List inUseRanges = getBlocking(AccordService.instance().node().commandStores().getInUseRangesAndMarkRetiredRangesUnsafeToRead()); - for (Ranges ranges : inUseRanges) - { - if (ranges.intersects(key)) - doesNotContainsToken = false; - } - return doesNotContainsToken; - }, 30); - }); - - NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); - - assertEquals(0, nodetoolResult.getRc()); - assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - // Ensure data is cleaned up - assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); - }); - } - - @Test - public void accordNodetoolCleanupPartialSSTableTest() throws Throwable - { - String tableName = "tbl1"; - String qualifiedTableName = KEYSPACE + '.' + tableName; - - test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { - cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); - cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 2, 2); - - SimpleQueryResult result1 = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 2 LIMIT 1", ConsistencyLevel.SERIAL); - SimpleQueryResult result2 = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); - - cluster.get(1).flush(withKeyspace("%s")); - assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - long token1 = (Long) result1.toObjectArrays()[0][0]; - long token2 = (Long) result2.toObjectArrays()[0][0]; - long newToken = token1 - 1000; - - assertTrue((token2 < newToken) && (token1 < Long.parseLong(originalToken))); - - assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - // Cluster 1 now only owns token2, but Accord still requires token1 - cluster.get(1).runOnInstance(() -> { - AccordService.instance().node().durability().shards().start(); - StorageService.instance.move(Long.toString(newToken)); - }); - - // Wait until Accord retires range, so it no longer has ownership of token1 - cluster.get(1).runOnInstance(() -> { - TableId tid = Schema.instance.getTableMetadata(KEYSPACE, tableName).id(); - RoutingKey key = TokenKey.parse(tid, String.valueOf(token1), Murmur3Partitioner.instance); - - Util.spinUntilTrue(() -> { - boolean doesNotContainsToken = true; - List inUseRanges = getBlocking(AccordService.instance().node().commandStores().getInUseRangesAndMarkRetiredRangesUnsafeToRead()); - for (Ranges ranges : inUseRanges) - { - if (ranges.intersects(key)) - doesNotContainsToken = false; - } - return doesNotContainsToken; - }, 30); - }); - - NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); - - assertEquals(0, nodetoolResult.getRc()); - assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - // Ensure data is correct - assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); - assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 2 LIMIT 1").length); - }); - - } - - @Test - public void accordNodetoolCleanupRangeInUseTest() throws Throwable - { - String tableName = "tbl2"; - String qualifiedTableName = KEYSPACE + '.' + tableName; - - test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { - cluster.coordinator(1).execute(wrapInTxn("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)"), ConsistencyLevel.SERIAL, 1, 2); - - SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); - - cluster.get(1).flush(withKeyspace("%s")); - - long token = (Long) result.toObjectArrays()[0][0]; - - assertTrue(token < Long.parseLong(originalToken)); - - assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - cluster.get(1).runOnInstance(() -> { - AccordService.instance().node().durability().shards().stop(); - StorageService.instance.move(Long.toString(token - 1000)); - }); - - NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); - - assertTrue(nodetoolResult.getStdout().contains("Some SSTables in keyspace " + KEYSPACE + " are still being used by Accord and were not cleaned up, check server logs for more information.")); - assertEquals(2, nodetoolResult.getRc()); - assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - // Ensure data is still there - assertEquals(1, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); - }); - } - - @Test - public void nodetoolCleanupForNonAccordTableTest() throws Throwable - { - String tableName = "tbl3"; - String qualifiedTableName = KEYSPACE + '.' + tableName; - - test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int)", cluster -> { - cluster.coordinator(1).execute("INSERT INTO " + qualifiedTableName + " (k, v) VALUES (?, ?)", ConsistencyLevel.ALL, 1, 2); - - SimpleQueryResult result = cluster.coordinator(1).executeWithResult("SELECT token(k) FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1", ConsistencyLevel.SERIAL); - - cluster.get(1).flush(withKeyspace("%s")); - - long token = (Long) result.toObjectArrays()[0][0]; - - assertTrue(token < Long.parseLong(originalToken)); - - assertEquals(1, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - cluster.get(1).runOnInstance(() -> { - AccordService.instance().node().durability().shards().stop(); - StorageService.instance.move(Long.toString(token - 1000)); - }); - - NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); - - assertEquals(0, nodetoolResult.getRc()); - assertEquals(0, (int) cluster.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore(tableName).getLiveSSTables().size())); - - // Ensure data is cleaned up - assertEquals(0, cluster.get(1).executeInternal("SELECT k FROM " + qualifiedTableName + " WHERE k = 1 LIMIT 1").length); - }); - } -} - From 4401aaa68662069ca96c1365675f3cfad06ab140 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Mon, 1 Jun 2026 15:58:41 -0400 Subject: [PATCH 26/27] fix test --- .../cassandra/distributed/test/accord/AccordCleanupTest.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java index a1175d38fe75..c569fe653e32 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java @@ -69,7 +69,7 @@ public static void setupClass() throws IOException AccordTestBase.setupCluster(builder -> builder .withoutVNodes() .appendConfig(config -> config - .set("accord.shard_durability_cycle", "25s") + .set("accord.shard_durability_cycle", "20s") .with(Feature.GOSSIP, Feature.NETWORK)), 2); SHARED_CLUSTER.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';'); SHARED_CLUSTER.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 1}"); @@ -97,7 +97,6 @@ public void accordNodetoolCleanupTest() throws Throwable String qualifiedTableName = KEYSPACE + '.' + tableName; test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { - // Loses ownership of k = 1 and Accord no longer needs it setupForCleanupTest(cluster, tableName, qualifiedTableName); NodeToolResult nodetoolResult = cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName); @@ -117,7 +116,6 @@ public void accordUserDefinedCleanupTest() throws Throwable String qualifiedTableName = KEYSPACE + '.' + tableName; test("CREATE TABLE " + qualifiedTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", cluster -> { - // Loses ownership of k = 1 and Accord no longer needs it setupForCleanupTest(cluster, tableName, qualifiedTableName); cluster.get(1).runOnInstance(() -> { From cc5911e211b9277f2e398ce612b385dc9b994b76 Mon Sep 17 00:00:00 2001 From: Alan Wang Date: Tue, 2 Jun 2026 14:54:23 -0700 Subject: [PATCH 27/27] reorder durability start/stop --- .../distributed/test/accord/AccordCleanupTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java index c569fe653e32..aaeecfb17c99 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java +++ b/test/distributed/org/apache/cassandra/distributed/test/accord/AccordCleanupTest.java @@ -260,8 +260,8 @@ private void setupForCleanupTest(Cluster cluster, String tableName, String quali // Cluster 1 no longer owns token cluster.get(1).runOnInstance(() -> { - StorageService.instance.move(Long.toString(token - 1000)); AccordService.instance().node().durability().shards().start(); + StorageService.instance.move(Long.toString(token - 1000)); }); // Wait until Accord retires range, so it no longer has ownership of token @@ -278,7 +278,7 @@ private void setupForCleanupTest(Cluster cluster, String tableName, String quali doesNotContainsToken = false; } return doesNotContainsToken; - }, 35); + }, 30); }); } @@ -304,8 +304,8 @@ private void setupForCleanupPartialSSTableTest(Cluster cluster, String tableName // Cluster 1 now only owns token2, but Accord still requires token1 cluster.get(1).runOnInstance(() -> { - StorageService.instance.move(Long.toString(newToken)); AccordService.instance().node().durability().shards().start(); + StorageService.instance.move(Long.toString(newToken)); }); // Wait until Accord retires range, so it no longer has ownership of token1 @@ -322,7 +322,7 @@ private void setupForCleanupPartialSSTableTest(Cluster cluster, String tableName doesNotContainsToken = false; } return doesNotContainsToken; - }, 35); + }, 30); }); }