Cleanup 15mar25 #3986

belliottsmith · 2025-03-15T19:46:03Z

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

Co-authored-by: Ariel Weisberg <[email protected]> Co-authored-by: Benedict Elliott Smith <[email protected]>

Introduce a special kind of non-durable read that provides only per-key linearizable isolation; i.e. strict-serializable isolation for single partition-key reads. This read creates only a happens-before edge, by collecting dependencies for execution and ensuring that execution happens strictly after these dependencies have executed, but at no precise time otherwise. So later writes may be witnessed, and if multiple keys are read they may represents different points in time. patch by Benedict; reviewed by Ariel Weisberg for CASSANDRA-19305

…endency elision patch by Benedict; reviewed by Aleksey Yeshchenko for CASSANDRA-19310

Patch by Blake Eggleston; Reviewed by Ariel Wesberg for CASSANDRA-19016

…nchmarks stable

patch by David Capwell; reviewed by Benedict Elliott Smith for CASSANDRA-19355

…d uses the sync point empty txn for reads patch by David Capwell; reviewed by Blake Eggleston for CASSANDRA-19503

…s to fail

…tdown causing the test to fail patch by David Capwell; reviewed by Blake Eggleston for CASSANDRA-19514

Patch by Blake Eggleston; Reviewed by Ariel Weisberg and David Capwell for CASSANDRA-19472

patch by Caleb Rackliffe; reviewed by David Capwell for CASSANDRA-18987

patch by Benedict Elliott Smith, David Capwell; reviewed by Benedict Elliott Smith for CASSANDRA-19605

…uring deserialization

…ct issues earlier before they are seen in Cassandra patch by Benedict Elliott Smith, David Capwell; reviewed by Benedict Elliott Smith, David Capwell for CASSANDRA-19618

patch by Aleksey Yeschenko; reviewed by Alex Petrov and Benedict Elliott Smith for CASSANDRA-18888

patch by David Capwell; reviewed by Blake Eggleston for CASSANDRA-19642

patch by Caleb Rackliffe; reviewed by David Capwell and Ariel Weisberg for CASSANDRA-18732

Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20393.

patch by Benedict Elliott Smith; reviewed by David Capwell for CASSANDRA-20420

…a NPE patch by David Capwell; reviewed by Benedict Elliott Smith for CASSANDRA-20417

Patch by Alex Petrov; reviewed by Benedict Elliott Smith CASSANDRA-20347.

Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20424.

… required topology changes patch by David Capwell; reviewed by Benedict Elliott Smith for CASSANDRA-20426

- Decouple command serialization from TableMetadata version; introduce ColumnMetadata ids; gracefully handle missing TableId - DataInputPlus.readLeastSignificantBytes must truncate high bits - Fix RandomPartitioner accord serialization - Fast path stable commits must not override recovery propose/commit decisions regarding visibility of a transaction - RejectBefore must mergeMax, not merge, to ensure we maintain epoch and hlc increasing independently - Bad commitInvalidate decision - consistent filtering for touches and stillTouches - ensure TRUNCATE_BEFORE implies SHARD_APPLIED - TopologyManager.unsyncedOnly off-by-one error - DurabilityQueue should not retry SyncPointErased - handle rare case of no deps but none needed - not updating CFK synchronously on recovery, which can lead to erroneous recovery decisions for other transactions - Don't return partial read response when one commandStore rejects the commit - Filter touches/stillTouches consistently - WaitingState computeLowEpoch must use hasTouched to handle historic key with no route Improve: - Use format parameters to defer building Invariants.requireArgument string - streamline RedundantStatus/RedundantBefore

Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20114

…e changes without impacting C* messagin patch by David Capwell; reviewed by Aleksey Yeschenko, Alex Petrov for CASSANDRA-20403

…ioner not being setup. The first example is the empty range so no updates to the static partitioner is done which leads to this NPE

- Accord Journal purging was disabled - remove unique_id from schema keyspace - avoid String.format in Compactor hot path - avoid string concatenation on hot path; improve segment compactor partition build efficiency - Partial compaction should update records in place to ensure truncation of discontiguous compactions do not lead to an incorrect field version being used - StoreParticipants.touches behaviour for RX was erroneously modified; should touch all non-redundant ranges including those no longer owned - SetShardDurable should correctly set DurableBefore Majority/Universal based on the Durability parameter - fix erroneous prunedBefore invariant - Journal compaction should not rewrite fields shadowed by a newer record - Don't save updates to ERASED commands - Simplify CommandChange.getFlags - fix handling of Durability for Invalidated - Don't use ApplyAt for GC_BEFORE with partial input, as might be a saveStatus >= ApplyAtKnown but with executeAt < ApplyAtKnown patch by Benedict; reviewed by Alex Petrov for CASSANDRA-20441

* Fix short accord simulation test (seed 0x6bea128ae851724b), ConcurrentModificationException * Increase wait time during closing to avoid Unterminated threads * Increase timeouts, improve test stability * More descriptive output from CQL test * Shorten max CMS delay * Improve future handling in config service Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20440

… ASYNC mode

dcapwell · 2025-03-27T16:09:17Z

test/unit/org/apache/cassandra/utils/concurrent/LockWithAsyncSignalTest.java

+import static java.util.concurrent.TimeUnit.NANOSECONDS;
+import static org.apache.cassandra.concurrent.ExecutorFactory.Global.executorFactory;
+
+public class LockWithAsyncSignalTest


wouldn't this be a better in Simulator? There is only one real "assert" in the test and its

FBUtilities.waitOnFutures(consumers, 2L, TimeUnit.SECONDS);

and environmental issues can cause this to not timeout within 2s leading it to be flakey. If we wish to debug why the test failed it wouldn't be reproducable.

The simulator is insufficient for fully testing this kind of construct, as it cannot simulate the memory model. The simulator does help validate systems like this, but we do need to also run it as the hardware intended.

I think as now designed it would be fine to increase this timeout arbitrarily high though. It would be nice to further embellish the testing of these constructs, but this is a fairly good investment:reward tradeoff (and at the moment in particular, everything is about balance of investment)

That said, this is lightweight enough I would hope 2s wasn’t unreasonable for any environment. Have you seen timeouts?

trying to fix repeat job to work with unit tests and on accord... that will help collect data about how stable this test is

$ junit_report_viewer.py All tests successful

1000 iterations with weaker resources and was stable, so guess we can leave this alone for now

dcapwell · 2025-03-27T16:15:03Z

src/java/org/apache/cassandra/service/accord/AccordExecutorLoops.java

 import static org.apache.cassandra.service.accord.AccordExecutor.Mode.RUN_WITH_LOCK;
 import static org.apache.cassandra.utils.Clock.Global.nanoTime;

-class AccordExecutorInfiniteLoops implements Shutdownable
+class AccordExecutorLoops


can you explain why you moved away from implements Shutdownable? During shutdown we don't look to directly tell this class anymore that we are shutting down, and we hope that each thread was told to shutdown; this feels like it should cause stability issues during nodetool drain and jvm-dtest shutdown no?

Looking closer this class is only used by: AccordExecutorAsyncSubmit, AccordExecutorSemiSyncSubmit, AccordExecutorSyncSubmit. So this class really doesn't care about directly being shutdown

dcapwell · 2025-03-27T16:19:31Z

src/java/org/apache/cassandra/service/accord/AccordExecutorLoops.java

+    private final AtomicInteger running = new AtomicInteger();
+    private final Condition terminated = Condition.newOneTimeCondition();
+
+    public AccordExecutorLoops(Mode mode, int threads, IntFunction<String> name, Function<Mode, Runnable> tasks)


given that this class no longer Loops this name feels off to me. We define that there are N threads, and we are done w/e all N finish running their task. This class doesn't really have anything to do with "looping", nor does it enforce that the sub-tasks are looped (old logic did enforce this)

This is arbitrary and to me it still makes perfect sense to call these threads “loops” since that’s what they do - we could if you like rename the loop provider from tasks to loops, if it helps your mental model.

dcapwell · 2025-03-27T21:03:01Z

src/java/org/apache/cassandra/service/accord/AccordExecutorAbstractLockLoop.java

+    public Object shutdownNow()
+    {
+        shutdown();
+        return null;


isn't the semantics that we don't to drop the backlog, yet this abstract class looks to push that to each implementation. In the locking case we only check if its shutdown if we get a null task.

I’m afraid I don’t follow. This could be changed really as we don’t implement Shutdownable, so we don’t need to return anything

i would remove this method to avoid confusion. shutdownNow normally clears the backlog, so if you see this method you assume it would as well.

My comment above was mistaken (done from a phone on holiday making assumptions about code location). This place does implement Shutdownable. Removing this therefore means not implementing Shutdownable. The description for shutdownNow in Shutdownable makes clear it is up to the implementation whether shutdownNow actually does anything to stop ongoing tasks, and this is true even for ExecutorService (though, the description there is that it is 'best effort', it makes clear there are no guarantees which is broadly equivalent to implementation defined)

I don't terribly mind not implementing Shutdownable though, if you're strongly of this view.

dcapwell · 2025-03-27T21:09:48Z

src/java/org/apache/cassandra/service/accord/AccordExecutorAbstractLockLoop.java

+    @Override
+    public void shutdown()
+    {
+        shutdown = true;


this isn't volatile so how do we make sure the visibility is correct for all versions?

here is what i see for AccordExecutorSyncSubmit

shutdown is called in random thread during shutdown

notifyWorkExclusive does hasWork.signal();

with lock case only checks this lock once we have no work to do, which is after we access this field

so from a JMM point of view this doesn't look like it actually forces the visibility?

Also, we don't block new work once we shutdown, so if the node is very busy this will keep going?

Visibility is enforced by other volatile accesses. The writer notifies work as you correctly surmised (which involves a volatile write), and the readers are all guarded by volatile reads on the lock they are guarded by (that we notify of work). The volatile keyword on this field would provide no guarantees on its own and offers no additional guarantees in concert with the lock.

it is true that we don’t reject new work but this executor should be shutdown after networking and we should not really reject any work being produced by the local machine during shutdown.

dcapwell · 2025-03-27T23:46:56Z

is there a reason this PR depends on apache/cassandra-accord#183? As far as i can tell they are unrelated?

dcapwell · 2025-03-31T22:43:31Z

Triggering CI to help validate the shutdown logic and the impact on CI. The shutdown semantic is the only thing really open so want to collect data on this.

dcapwell · 2025-03-31T23:02:57Z

Ran CI and the shutdown logic is causing tests to fail, we don't close the cluster within 3m

belliottsmith · 2025-04-01T10:12:13Z

is there a reason this PR depends on apache/cassandra-accord#183? As far as i can tell they are unrelated?

They are related in the sense that they each represent the latest batch of significant fixes for either side. It just so happens this time that it appears the two sides do not actually intersect. I will update the description and remove the dependency.

dcapwell · 2025-04-03T21:13:26Z

src/java/org/apache/cassandra/service/accord/AccordExecutorLoops.java

@@ -24,7 +24,7 @@
 import java.util.function.IntFunction;

 import accord.utils.Invariants;
-import org.agrona.collections.LongHashSet;
+import io.netty.util.collection.LongObjectHashMap;


curious why you moved off agrona to netty's. Not against it, just wondering if there was a reason

Mistake - intended only to switch to a map to avoid unlikely scenario of thread if being reused for a dead thread (close to impossible given how we use it, but safer). Will switch to Agrona map version for consistency

dcapwell · 2025-04-03T21:13:39Z

src/java/org/apache/cassandra/service/accord/AccordExecutorAbstractLockLoop.java

-        notifyWorkExclusive();
+        notifyWork();
+        lock.lock();
+        try


think this try/finally isn't needed?

Oh yes, leftover it looks like, not sure how that happened

belliottsmith and others added 30 commits September 19, 2024 15:09

CASSANDRA-18365: Protocol fixes

163e491

Fix SerializerSupport.reconstruct, and introduce RangeFactory

7445a29

Get simulator working (again)

aed8e5c

Co-authored-by: Ariel Weisberg <[email protected]> Co-authored-by: Benedict Elliott Smith <[email protected]>

Fix (de)serialization of WaitingOn into cache

c32390f

Fix simulator logging and clean shutdown

eeb687a

Add TODO for TxnQuery using wrong cluster metadata

3ae47b3

Refactor CommandsForKey for efficiency, and to support transitive dep…

33294ed

…endency elision patch by Benedict; reviewed by Aleksey Yeshchenko for CASSANDRA-19310

CEP-15: (C*) per-table transactional configuration

ba2d08d

Patch by Blake Eggleston; Reviewed by Ariel Wesberg for CASSANDRA-19016

fix MultiElementType pack/unpack accessor api

f80a4d4

post-trunk rebase fixes

ea7a355

perf improvements

8e24a8f

Ninja for CASSANDRA-19305: Disable EphemeralRead by default to get be…

7995339

…nchmarks stable

Accord: PreLoadContext must properly and consistently support ranges

2f89946

patch by David Capwell; reviewed by Benedict Elliott Smith for CASSANDRA-19355

fix ReadCommand.serializedSize

cca3d1f

revert async message delivery

338e282

(Accord) Cassandra bootstrap no longer using the range txn and instea…

de3703f

…d uses the sync point empty txn for reads patch by David Capwell; reviewed by Blake Eggleston for CASSANDRA-19503

AccordGens.rangeDeps did not enforce unique ranges, which caused test…

c382715

…s to fail

fix read repair tests

8ca819b

When jvm-dtest is shutting down an instance TCM retries block the shu…

e4a5a1a

…tdown causing the test to fail patch by David Capwell; reviewed by Blake Eggleston for CASSANDRA-19514

CEP-15 (C*) Integrate accord with repair

f4e012a

Patch by Blake Eggleston; Reviewed by Ariel Weisberg and David Capwell for CASSANDRA-19472

Prohibit counter column access in Accord transactions

48815b1

patch by Caleb Rackliffe; reviewed by David Capwell for CASSANDRA-18987

Accord: NPE in RangeDeps.forEach

caa5381

patch by Benedict Elliott Smith, David Capwell; reviewed by Benedict Elliott Smith for CASSANDRA-19605

post-rebase fixes, mostly around CASSANDRA-19341 and CASSANDRA-19567

ac8a0c1

make sure StreamSummarySerializer uses the appropriate IPartitioner d…

baf7759

…uring deserialization

changing accord submodule branch back to trunk

39ea0ca

Accord: Need to simulate Cassandra Journal in Accord BurnTest to dete…

5a4c726

…ct issues earlier before they are seen in Cassandra patch by Benedict Elliott Smith, David Capwell; reviewed by Benedict Elliott Smith, David Capwell for CASSANDRA-19618

Move preaccept expiration logic away from Agent

0cacea2

patch by Aleksey Yeschenko; reviewed by Alex Petrov and Benedict Elliott Smith for CASSANDRA-18888

IndexOutOfBoundsException while serializing CommandsForKey

4d4fee8

patch by David Capwell; reviewed by Blake Eggleston for CASSANDRA-19642

Baseline Diagnostic vtables for Accord

0631732

patch by Caleb Rackliffe; reviewed by David Capwell and Ariel Weisberg for CASSANDRA-18732

ifesdjeen and others added 16 commits March 4, 2025 16:44

Add initial Accord Overview Doc

63ec070

Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20393.

InterceptingExecutor.schedule returns a simulator-unsafe Future

6f200ec

patch by Benedict Elliott Smith; reviewed by David Capwell for CASSANDRA-20420

When bootstrap fails the retry field is not populated which leads to …

7eff02d

…a NPE patch by David Capwell; reviewed by Benedict Elliott Smith for CASSANDRA-20417

Epoch/Topology Garbage Collection

b3a9b98

Patch by Alex Petrov; reviewed by Benedict Elliott Smith CASSANDRA-20347.

Implement Nemesis compactor, improve partial compaction

3be0d93

Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20424.

Topology mixup tests should do repairs for host replacement and other…

92d90a8

… required topology changes patch by David Capwell; reviewed by Benedict Elliott Smith for CASSANDRA-20426

Accord repin: turn pruning back on

9d4b538

Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20114

Decouple Accord serializers from C* message version so accord can mak…

29dc5a4

…e changes without impacting C* messagin patch by David Capwell; reviewed by Aleksey Yeschenko, Alex Petrov for CASSANDRA-20403

Ninja for CASSANDRA-20403: KeySerializersTest would NPE due to partit…

3b9b866

…ioner not being setup. The first example is the empty range so no updates to the static partitioner is done which leads to this NPE

- switch to plain Thread for AccordExecutor, and improve testing for…

e3e38a8

… ASYNC mode

- Cheaper loggingId

2e80d41

update accord

8592a70

update accord

3d99210

dcapwell changed the base branch from trunk to cep-15-accord March 21, 2025 17:58

dcapwell reviewed Mar 27, 2025

View reviewed changes

fix shutdown notify

789ab16

asfgit force-pushed the cep-15-accord branch from 8d112e8 to 5ea9486 Compare April 2, 2025 16:36

dcapwell force-pushed the cep-15-accord branch 2 times, most recently from 7be53d5 to de9925f Compare April 2, 2025 21:37

dcapwell reviewed Apr 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup 15mar25 #3986

Cleanup 15mar25 #3986

belliottsmith commented Mar 15, 2025

dcapwell Mar 27, 2025

belliottsmith Mar 27, 2025

belliottsmith Mar 27, 2025

dcapwell Mar 27, 2025

dcapwell Mar 27, 2025 •

edited

Loading

dcapwell Mar 27, 2025

dcapwell Mar 27, 2025

dcapwell Mar 27, 2025

belliottsmith Mar 28, 2025

dcapwell Mar 27, 2025

belliottsmith Mar 28, 2025

dcapwell Mar 31, 2025

belliottsmith Apr 1, 2025

dcapwell Mar 27, 2025

dcapwell Mar 27, 2025

dcapwell Mar 27, 2025

belliottsmith Mar 28, 2025 •

edited

Loading

dcapwell commented Mar 27, 2025

dcapwell commented Mar 31, 2025

dcapwell commented Mar 31, 2025

belliottsmith commented Apr 1, 2025

dcapwell Apr 3, 2025

belliottsmith Apr 3, 2025

dcapwell Apr 3, 2025

belliottsmith Apr 3, 2025

Cleanup 15mar25 #3986

Are you sure you want to change the base?

Cleanup 15mar25 #3986

Conversation

belliottsmith commented Mar 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcapwell Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belliottsmith Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

dcapwell commented Mar 27, 2025

dcapwell commented Mar 31, 2025

dcapwell commented Mar 31, 2025

belliottsmith commented Apr 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcapwell Mar 27, 2025 •

edited

Loading

belliottsmith Mar 28, 2025 •

edited

Loading