CommandChanges.shouldCleanup short-circuits to NO if there is no data… by belliottsmith · Pull Request #4873 · apache/cassandra

belliottsmith · 2026-06-08T11:21:47Z

…, but this is incorrect as Cleanup.EXPUNGE may have dropped the data and the record must receive cleanup EXPUNGE and be reported as ERASED. This can lead to lienarizability violations.

Thanks for sending a pull request! Here are some tips if you're new here:

Ensure you have added or run the appropriate tests for your PR.
Be sure to keep the PR description updated to reflect all changes.
Write your PR title to summarize what this PR proposes.
If possible, provide a concise example to reproduce the issue for a faster review.
Read our contributor guidelines
If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

alanwang67 · 2026-06-10T19:54:55Z

+1

Copilot

Pull request overview

This PR aims to prevent Accord journal cleanup/expunge decisions from being surfaced as NotDefined on the load path (which the PR description notes can cause linearizability violations). The visible changes in this repo primarily add/adjust tests and test utilities to reproduce and guard the behavior.

Changes:

Add a new regression unit test (AccordExpungeTest) covering Cleanup.EXPUNGE interactions with CommandChanges reconstruction and journal.loadCommand.
Extend AccordGenerators.commandsBuilder() with an overload to allow custom TxnId generation (used by the new test).
Adjust distributed burn-test behavior and minor test cleanup/comment removals.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`test/unit/org/apache/cassandra/utils/AccordGenerators.java`	Adds an overload to build commands with a caller-supplied `TxnId` generator.
`test/unit/org/apache/cassandra/service/accord/AccordExpungeTest.java`	New regression test exercising EXPUNGE → loadCommand behavior across journal states.
`test/distributed/org/apache/cassandra/service/accord/journal/AccordJournalBurnTest.java`	Sets a global burn-test tuning parameter (`CMD_BASE_CHECK_CHANCE`) via static initializer.
`test/distributed/org/apache/cassandra/service/accord/AccordJournalCompactionTest.java`	Minor whitespace-only change.
`test/distributed/org/apache/cassandra/distributed/test/accord/AccordLoadTest.java`	Removes commented-out config lines.
`.gitmodules`	Points the `modules/accord` submodule at a personal fork/branch instead of the canonical Apache repo.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…, but this is incorrect as Cleanup.EXPUNGE may have dropped the data and the record must receive cleanup EXPUNGE and be reported as ERASED. This can lead to lienarizability violations.

…s where the ownership is unknown

- system_accord_debug.executors has wrong clustering type - RemoteToLocalVirtualTable should lazily allocate the partition collector to avoid LIMIT clause filtering removing it before it's populated

- Ensure ReadCoordinator callbacks are invoked on owning thread

… tryExecuteListening terminates

… as soon as we have data

ifesdjeen · 2026-06-16T07:05:36Z

    {
        if (cell.column().isSimple())
-            return new BTreeRow(clustering, BTree.singleton(cell), minDeletionTime(cell));
+            return new BTreeRow(clustering, primaryKeyLivenessInfo, Deletion.LIVE, BTree.singleton(cell), minDeletionTime(cell));


Do we want to do

long minDeletionTime = Math.min(minDeletionTime(primaryKeyLivenessInfo), minDeletionTime(deletion.time()));

as we do in create above here too?

ifesdjeen · 2026-06-16T07:10:23Z

+                        Future<?> future = toFuture(AsyncResults.reduce(results, Reduce.toNull()));
+                        long timeoutSeconds = getAccord().execute_waiting_on_start_timeout.toSeconds();
+                        if (timeoutSeconds <= 0) future.awaitUninterruptibly().rethrowIfFailed();
+                        else if (!future.awaitUninterruptibly(timeoutSeconds, SECONDS))


I think we would throw here only on timeout, but not on failure; should we add future.isSuccess() too?

Weird, I thought I already handled that. Thanks!

ifesdjeen requested review from Copilot and ifesdjeen June 11, 2026 13:20

ifesdjeen approved these changes Jun 11, 2026

View reviewed changes

Copilot started reviewing on behalf of ifesdjeen June 11, 2026 13:20 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

Comment thread test/unit/org/apache/cassandra/service/accord/AccordExpungeTest.java

Comment thread test/unit/org/apache/cassandra/service/accord/AccordExpungeTest.java

Comment thread .gitmodules

Comment thread test/distributed/org/apache/cassandra/service/accord/journal/AccordJournalBurnTest.java

belliottsmith force-pushed the consistent-expunge branch 9 times, most recently from f021f32 to 00714e8 Compare June 14, 2026 17:46

belliottsmith added 14 commits June 15, 2026 13:25

CommandChanges.shouldCleanup short-circuits to NO if there is no data…

0f5130e

…, but this is incorrect as Cleanup.EXPUNGE may have dropped the data and the record must receive cleanup EXPUNGE and be reported as ERASED. This can lead to lienarizability violations.

test

7ea2dc5

Update RangesForEpoch directly, so that we cannot have race condition…

71c3d90

…s where the ownership is unknown

Fix:

7f292c5

- system_accord_debug.executors has wrong clustering type - RemoteToLocalVirtualTable should lazily allocate the partition collector to avoid LIMIT clause filtering removing it before it's populated

- Avoid reentrancy on local callbacks

c516f85

- Ensure ReadCoordinator callbacks are invoked on owning thread

- Avoid deadlock when notifying ComplexListener(s)

0483004

- Wait a maximum of 5m for pending transaction execution

710649f

fix

1039fae

maybeExecute must invoke either notWaiting or notifyWaiting to ensure…

088e6e0

… tryExecuteListening terminates

execute_waiting_on_start configurable

60edb2a

Fix JournalBurnTest validation

a38ed53

Populate CFK system table row markers so we can stop reading sstables…

f4a4341

… as soon as we have data

improvements

c98ef9c

cleanup

f2b5980

belliottsmith force-pushed the consistent-expunge branch from fec9c89 to f2b5980 Compare June 15, 2026 12:26

ifesdjeen approved these changes Jun 16, 2026

View reviewed changes

feedback

4f8436c

belliottsmith force-pushed the consistent-expunge branch from 0c7d366 to 4f8436c Compare June 16, 2026 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CommandChanges.shouldCleanup short-circuits to NO if there is no data…#4873

CommandChanges.shouldCleanup short-circuits to NO if there is no data…#4873
belliottsmith wants to merge 15 commits into
apache:trunkfrom
belliottsmith:consistent-expunge

belliottsmith commented Jun 8, 2026

Uh oh!

alanwang67 commented Jun 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ifesdjeen Jun 16, 2026

Uh oh!

belliottsmith Jun 16, 2026

Uh oh!

ifesdjeen Jun 16, 2026

Uh oh!

belliottsmith Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

belliottsmith commented Jun 8, 2026

Uh oh!

alanwang67 commented Jun 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ifesdjeen Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

belliottsmith Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

ifesdjeen Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

belliottsmith Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants