CASSANDRA-19433 Nodetool cleanup can drop data Accord might need by alanwang67 · Pull Request #4692 · apache/cassandra

alanwang67 · 2026-03-25T01:15:35Z

Checks that range for SSTables being cleaned up is not still being used by Accord CommandStores prior to cleaning it up. apache/cassandra-accord#270 needs to be merged prior.

patch by Alan Wang;

reviewed by for CASSANDRA-19433

Co-authored-by: Name1
Co-authored-by: Name2

… deleted by nodetool cleanup

aweisberg

This is partial feedback, have to get to a meeting, but this should unblock some progress.

aweisberg · 2026-04-08T19:32:06Z

+        if (!AccordService.isSetup())
+            return false;
+
+        Future<List<Ranges>> futureAccordOwnedRanges = AccordService.toFuture(AccordService.instance().node().commandStores().getInUseRanges());


This is going to fetch the in use ranges for every single sstable of which there are thousands. It should probably fetch them once at the start and then keep referring to it. Also if the usage here is synchronous then the Accord method should also be synchronous or have a synchronous version you can call so that the boilerplate of getting the async value is handled inside the helper function.

aweisberg · 2026-04-08T19:55:58Z

+            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()));


Do you actually need to wrap this in Ranges in order to test intersection?

aweisberg · 2026-04-08T19:59:17Z

                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


This is parallelAllSSTablesOperation and cleanup is just one kind of thing it might do?

I see, though we can only have an incomplete operation with cleanup, but let me fix the comment to be more general. Do we want to extend this to the other operations as well?

aweisberg · 2026-04-08T20:03:03Z

                    boolean needsCleanupTransient = !transientRanges.isEmpty() && sstable.isRepaired() && needsCleanup(sstable, transientRanges);
+                    totalSSTables++;
+
+                    if (sstableContainsRangesNeededByAccord(cfStore.getTableId(), sstable))


This entirely excludes sstables from cleanup assuming they are entirely needed by Accord when in reality it's just an intersecting section that is needed. Cleanup will actually rewrite sstables to remove the parts that can be cleaned up.

Being unable to clean up an sstable doesn't represent an incomplete operation unless the table is also no longer owned? Just because Accord needs it doesn't mean we were going to clean it up. Unless I am missing something earlier about already having filtered the sstables to be the ones intersecting non-owned ranges. I'm not sure if we are iterating all or just some.

I see, I think what I will do instead is to just add the range that Accord is using to the full range. This way when cleanup is done we will still remove ranges from the SSTables even if Accord partially owns a piece of it and we can just remove this if statement all together.

…o owned ranges and let existing logic handle

aweisberg

There is a lot going on here that I need to figure out with transient ranges. The correct behavior for transient ranges changed with new witnesses and mutation tracking.

I'm also really not clear on what doCleanupOne is doing tracking incomplete based on Accord owned ranges. I guess what it is doing is reported that a row actually intersected with an Accord owned range? I don't think we need to do that on a per row basis it can be done once in filterSSTables. If any sstable intersects an exclusively Accord owned range (not in the write placement ranges) we know cleanup will be incomplete because we expanded the set of owned ranges to include the Accord owned ranges.

aweisberg · 2026-04-09T20:16:07Z

        ABORTED(1),
-        UNABLE_TO_CANCEL(2);
+        UNABLE_TO_CANCEL(2),
+        INCOMPLETE(3);


Nodeprobe doesn't handle this. Cleanup should signal an error if some data wasn't cleaned up since what was requested didn't fully happen.

aweisberg · 2026-04-09T20:18:06Z

            }
+
+            @Override
+            public boolean incompleteOperation()


Use a default method to avoid having to add this everywhere

aweisberg · 2026-04-09T20:20:15Z

            {
                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);


This overwrites whether it was complete for each sstable so the final value is just what happened to the last sstable. You also don't need this?

Got it, will remove this and perform this in filter operations.

aweisberg · 2026-04-09T20:23:44Z

+        }
+
+        final Set<Range<Token>> allRanges = Stream.concat(localWrites.ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet());
+        final Set<Range<Token>> transientRanges = Stream.concat(localWrites.onlyTransient().ranges().stream(), rangesInUseByAccord.stream()).collect(Collectors.toSet());


Accord doesn't support transient replication so it should really be a checkState erroring out if we are using Accord and there are transient ranges. Either way for transient ranges Accord may actually be using them even when there is cleanup work to do to remove them. The check would be more complicated because we would need Accord to report whether it is using the range transiently or fully and that distinction doesn't exist yet.

So that is why checkState and then don't expand the transient ranges to include Accord owned ranges. Just leave it as is.

aweisberg · 2026-04-09T22:34:04Z

    }

+    @Override
+    public List<Ranges> getInUseRanges()


getInUseRanges treats the ranges Accord manages as being global across tables, but it's actually a per table property. This should be a map from TableId to a collection of ranges.

… signature

aweisberg

Almost there, I have a feeling this will be the last round :-)

aweisberg · 2026-04-16T16:23:13Z


-        final Set<Range<Token>> allRanges = new HashSet<>(localWrites.ranges());
+        Set<Range<Token>> noLongerOwnedRangesInUseByAccord = new HashSet<>();
+        if (AccordService.isSetup())


Accord can be set up and it's fine. It's what is going on with this specific table that matters since whether Accord is enabled is on a per table basis.

It should also be a checkState since it's an invariant that should already be being enforced.

aweisberg · 2026-04-16T16:26:39Z

            {
                CleanupStrategy cleanupStrategy = CleanupStrategy.get(cfStore, allRanges, transientRanges, txn.onlyOne().isRepaired(), FBUtilities.nowInSeconds());
-                doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes);
+                if (doCleanupOne(cfStore, txn, cleanupStrategy, allRanges, hasIndexes, noLongerOwnedRangesInUseByAccord))


The "if" isn't needed technically. It could be an |=, but that is me being super pedantic about style.

aweisberg · 2026-04-16T16:28:33Z

+        Set<Range<Token>> noLongerOwnedRangesInUseByAccord = new HashSet<>();
+        if (AccordService.isSetup())
+        {
+            if (!localWrites.onlyTransient().ranges().isEmpty())


Specifically this is too strict since it doesn't check if the transient ranges are on an Accord enabled table. You also technically don't need to do this check here because it should be enforced during schema change, but it's nice to have.

aweisberg · 2026-04-16T16:32:27Z

+            }
+        }
+
+        final Set<Range<Token>> allRanges = Stream.concat(localWrites.ranges().stream(), noLongerOwnedRangesInUseByAccord.stream()).collect(Collectors.toSet());


The choice of set as a data structure is just odd because Ranges don't work well as sets since it still allows overlaps and doesn't normalize. This whole section would be a little more sane if we used NormalizedRanges as the final storage, but that might be finicky so for this make sure to Normalize the ranges before constructing the final set so it's at least a sane looking set of ranges.

aweisberg · 2026-04-16T18:08:25Z

+
+            cluster.get(1).runOnInstance(() -> StorageService.instance.move(Long.toString(token - 1000)));
+
+            cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName);


You really want to "pause" something to keep Accord from retiring the range while cleanup runs so you can validate the retention happens. That would allow having a reduced timeout and shared cluster. If the timeout is a hot property you could update it for just this test or make it a hot property and rely on that instead of pausing or just have this one test create its own cluster with a longer timeout instead of using the shared cluster.

On this pass of review I am missing how this issue was resolved?

Also this nodetool result is not validated.

Just resolved this, prior to moving the token I make sure to either turn off or on ShardDurability depending on the test case, also lowered the default shard_durability_cycle to be 1s for the test run faster.

Had to increase the shard_durability_cycle, I think there's some test flakiness when setting it too low.

aweisberg

I found some material issues. Also this doesn't pass style checks.

aweisberg · 2026-05-14T21:27:03Z


        return parallelAllSSTableOperation(cfStore, new OneSSTableOperation()
        {
+            boolean incompleteOperation = false;


I made a mistake. This needs to be an atomic boolean and be CASed. The |= could turn an incompleteOperation that is already set to true to false.

aweisberg · 2026-05-14T21:28:57Z

+        List<Range<Token>> noLongerOwnedRangesInUseByAccord = new ArrayList<>();
+        TableMetadata metadata = Schema.instance.getTableMetadata(tableId);
+
+        if (metadata != null && metadata.isAccordEnabled())


This is not migration aware. We need to keep the table when Accord is in the process of being enabled, when it is enabled, and when it is in the process of being disabled.

aweisberg · 2026-05-14T21:40:31Z

+
+            cluster.get(1).runOnInstance(() -> StorageService.instance.move(Long.toString(token - 1000)));
+
+            cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName);


On this pass of review I am missing how this issue was resolved?

aweisberg · 2026-05-14T21:40:54Z

+
+            cluster.get(1).runOnInstance(() -> StorageService.instance.move(Long.toString(token - 1000)));
+
+            cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName);


Also this nodetool result is not validated.

aweisberg

forceUserDefinedCleanup also needs tests

alanwang67 added 9 commits March 24, 2026 16:25

added check to make sure ranges that are still used by Accord are not…

32bec7b

… deleted by nodetool cleanup

special case sstable with single key

b072804

removed unused imports

e3bd0f8

fix up tests

3d752e3

comments

b20db8f

moved test to cql test base

5a892a4

add additional test and check if accord service is on

ecbf66f

cleanup on retirement rather than on command store deletion

33ecb95

refactor tests

b5934f8

aweisberg requested changes Apr 8, 2026

View reviewed changes

alanwang67 added 2 commits April 8, 2026 15:43

Rather than checking SSTable intersection, add in use Accord Ranges t…

d826bee

…o owned ranges and let existing logic handle

added incomplete operation

804e6dd

aweisberg requested changes Apr 9, 2026

View reviewed changes

alanwang67 added 6 commits April 12, 2026 22:38

Handle nodeprobe + add check for transient range + change inUseRanges…

5742a4c

… signature

remove new line

6e55ae1

fix bug

b0311b8

set incompleteOperation based off iterating SSTables

564e227

removed lock

cc5c8a1

Accord module changes

f4b283c

aweisberg requested changes Apr 16, 2026

View reviewed changes

addressed comments

62ce86a

aweisberg requested changes May 14, 2026

View reviewed changes

alanwang67 added 4 commits May 14, 2026 21:29

fixes

dd79e00

fix test flakiness

528c432

fix import order

abc708d

mark unsafe to read for retired ranges we are cleaning up

fdedf38

aweisberg requested changes May 28, 2026

View reviewed changes

Comment thread src/java/org/apache/cassandra/db/compaction/CompactionManager.java Outdated

Comment thread src/java/org/apache/cassandra/db/compaction/CompactionManager.java Outdated

Comment thread test/distributed/org/apache/cassandra/distributed/test/accord/AccordNodetoolCleanupTest.java Outdated

alanwang67 added 3 commits June 1, 2026 13:17

fix comment

fe539d9

fix test

42c6b46

include tests for force user defined cleanup

3c4a9f4

alanwang67 added 2 commits June 1, 2026 15:58

fix test

4401aaa

reorder durability start/stop

cc5911e


		cluster.get(1).runOnInstance(() -> StorageService.instance.move(Long.toString(token - 1000)));

		cluster.get(1).nodetoolResult("cleanup", KEYSPACE, tableName);

Uh oh!

Conversation

alanwang67 commented Mar 25, 2026

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanwang67 Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

alanwang67 Apr 8, 2026 •

edited

Loading