CASSANDRA-19744 Accord migration and interop correctness #3395

aweisberg · 2024-07-02T16:32:49Z

CASSANDRA-19744

There are several different fixes/changes here.

Batch log replay splits mutations based on whether they should be Accord/non-Accord routed
Failed batchlog replay of Accord txns are sent to hints as a special host ID and hints replays them and if necessary converts them back to per host hints, per host hints are never converted to Accord hints so the amplification is bounded in the rare case this occurs.
Hints split and retry Accord/non-Accord and handle Accord tens with no specific endpoint just for the batchlog
Batch log is skipped if the entire batch is routed to Accord, if the part of the batch is non-Accord then he entire batch goes through the batchlog
Barriers now return the precise Accord owned ranges that were barriered which may be a subset of what was requested
Repair now drives progress for migration based on what was actually barriered
Read repair routes the repair mutation based on whether the token is Accord managed, but does not retry if it fails. We just fail the read since this should be rare.
Regular mutations will loop splitting and retrying until timeout or until the routing is correct.
Mutations now have a flag that indicates whether they bypass validation for a conflict with a transaction system, Accord always sets this flag since it doesn't conflict with itself. Everything else doesn't and a CFS will reject a mutation that should have been managed by Accord. Counter and virtual tables don't use this since they don't work with Accord right now.
Timestamps of non-transactional writes are preserved in that the timestamp that is used by Accord to write data is the minimum of the Accord timestamp and the original coordinator generated timestamp to avoid data resurrection from batch and hints applied at a later time. CAS, Accord transactions, and Read Repair don't preserve timestamps.
Fixed handling of non-existent tables and tables that are system tables where we would NPE because not all schema sources contain non-distributed tables.
Fixed registration race conditions in TestChangeListener pausing enactment of epochs
Fixed cep-15-accord specific breakage of handling CoordinatorBehind responses
CoordinatorBehind errors no longer fail operations, the coordinator will retry

~~TODO CoordinatorBehind retry could retry operations that are not idempotent, but may have already succeeded~~
~~TODO Check that barriers of keys check the key was actually barriered~~
TODO The batchlog failed routing test doesn't actually experience failed routing it's also a timeout, but it should be possible to trigger a failed routing by doing a transaction on the key and then allowing the out of sync coordinator to catch up and be forced to execute the transaction in the new epoch.

…ally split and reroute if necessary Mostly complete batch + hint support checkpoint here checkpoint again accord branch more checkpoint More checkpoint Checkpoint checkpoint checkpoint fixes Update .gitmodules, make CASMultiDCTest loopable

…failure, various doc and bug fixes:

bdeggleston · 2024-07-16T18:53:57Z

src/java/org/apache/cassandra/service/accord/api/AccordAgent.java

@@ -122,7 +122,7 @@ public long preAcceptTimeout()
    @Override
    public Txn emptyTxn(Kind kind, Seekables<?, ?> seekables)
    {
-        return new Txn.InMemory(kind, seekables, TxnRead.EMPTY, TxnQuery.EMPTY, null);
+        return new Txn.InMemory(kind, seekables, TxnRead.EMPTY, TxnQuery.UNSAFE_EMPTY, null);


could we rename this method to emptySystemTxn or emptyUnsafeTxn or something so people know that it doesn't offer all the normal guardrails

bdeggleston · 2024-07-16T23:03:35Z

src/java/org/apache/cassandra/db/CassandraKeyspaceWriteHandler.java

@@ -89,7 +89,7 @@ private CommitLogPosition addToCommitLog(Mutation mutation)
                    if (update.metadata().params.memtable.factory().writesShouldSkipCommitLog())
                        ids.add(update.metadata().id);
                }
-                mutation = mutation.without(ids);
+                mutation = mutation.filter(ids::contains);


This doesn't seem right. If I'm reading this right then when we encounter mixed writesShouldSkipCommitLog mutations we'll only write commit log entries for tables which aren't supposed to be written to the commit log?

bdeggleston · 2024-07-16T23:07:36Z

src/java/org/apache/cassandra/db/IMutation.java

+     * like Accord that can't safely read data that is written non-transactionally.
+     *
+     */
+    default boolean allowPotentialTransactionConflicts()


I had a hard time parsing this comment
TODO: propose something easier to understand

bdeggleston · 2024-07-17T00:12:39Z

test/unit/org/apache/cassandra/cql3/PagingTest.java

@@ -140,7 +140,7 @@ public boolean isWorthMergingForRangeQuery(ReplicaCollection merged, ReplicaColl
        }
        Thread.sleep(1500);

-        Statement stmt = new SimpleStatement(String.format("SELECT DISTINCT token(id, id2), id, id2 FROM %s", table));
+        Statement stmt = new SimpleStatement(String.format("SELECT DISTINCT token(id, id2), id, id2 FROM %s WHERE token(id, id2) >= -9223372036854775808", table));


what's up with the token bit you added here?

bdeggleston · 2024-07-17T18:03:44Z

src/java/org/apache/cassandra/batchlog/BatchlogManager.java

        }

-        private int addMutations(int version, List<ByteBuffer> serializedMutations) throws IOException
+        private int addMutations(List<Mutation> unsplitMutations, int version, List<ByteBuffer> serializedMutations) throws IOException


this (and addMutation) can be made static if you pass writtenAt in

bdeggleston · 2024-07-22T17:02:44Z

src/java/org/apache/cassandra/hints/HintsDispatcher.java

+            Mutation mutation = hint.mutation;
+            // Also may need to apply locally because it's possible this is from the batchlog
+            // and we never applied it locally
+            // TODO (review): Additional error handling necessary? Hints are lossy


agreed, they're best effort

bdeggleston · 2024-07-22T17:05:48Z

src/java/org/apache/cassandra/hints/HintsDispatcher.java

+            this.accordTxnStartNanos = accordTxnStartNanos;
+            this.accordTxnResult = accordTxnResult;
+            if (accordTxnResult != null)
+                accordTxnResult.right.addListener(this, ImmediateExecutor.INSTANCE);


shouldn't we register as a result/throwable consumer so we are notified if the txn fails unexpectedly?

bdeggleston · 2024-07-22T17:21:43Z

src/java/org/apache/cassandra/service/consensus/TransactionalMode.java

@@ -112,6 +112,10 @@ public ConsistencyLevel commitCLForStrategy(ConsistencyLevel consistencyLevel)
        return consistencyLevel;
    }

+    // TODO (required): This won't work for migration directly from none to full because there is no safe system to read from


What a migration from none to full effectively ran accord in mixed_reads mode during migration?

bdeggleston · 2024-07-22T17:23:34Z

src/java/org/apache/cassandra/service/AbstractWriteResponseHandler.java

@@ -294,8 +329,20 @@ public void onFailure(InetAddressAndPort from, RequestFailure failure)
        if (blockFor() + n > candidateReplicaCount())
            signal();

-        if (hintOnFailure != null && StorageProxy.shouldHint(replicaPlan.lookup(from)))
-            StorageProxy.submitHint(hintOnFailure.get(), replicaPlan.lookup(from), null);
+        // If the failure was RETRY_ON_DIFFERENT_TRANSACTION_SYSTEM then we only want to hint once


wouldn't we want to retry with accord instead of writing a hint?

bdeggleston · 2024-07-22T17:31:03Z

src/java/org/apache/cassandra/service/consensus/migration/ConsensusMigrationMutationHelper.java

+            throw new RetryOnDifferentSystemException();
+    }
+
+    public static boolean tokenShouldBeWrittenThroughAccord(@Nonnull ClusterMetadata cm, @Nonnull TableMetadata tm, @Nonnull Token token)


We should be passing in the TableId and using it to retrieve the table metadata from the provided ClusterMetadata. Passing in table metadata directly like this makes it possible to read table params from a different epoch than the cm passed in, which I think is possible in invocation in BlockingReadRepair

aweisberg added 3 commits July 2, 2024 12:11

Fix routing of read repair to the correct system based on token

11d21a1

Preserve non-transactional mutation timestamps

6a596e5

aweisberg requested review from bdeggleston, dcapwell and belliottsmith July 2, 2024 16:32

aweisberg added 2 commits July 2, 2024 17:57

Part of David's review feedback

b0674e1

For elapsed time use a monotonic source

9aa647d

aweisberg force-pushed the cassandra-19744 branch from a4fcbd8 to 17b6f85 Compare July 5, 2024 16:42

More review feedback

a4468ad

aweisberg force-pushed the cassandra-19744 branch from 17b6f85 to a4468ad Compare July 5, 2024 17:01

aweisberg added 2 commits July 5, 2024 13:15

Roll back Dispatcher changes because they don't hanadle idempotency

0d9b748

Migrating away from accord test routing failure as an actual routing …

96a7efd

…failure, various doc and bug fixes:

aweisberg force-pushed the cassandra-19744 branch from 7245fc8 to 96a7efd Compare July 9, 2024 18:30

bdeggleston requested changes Jul 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSANDRA-19744 Accord migration and interop correctness #3395

CASSANDRA-19744 Accord migration and interop correctness #3395

aweisberg commented Jul 2, 2024 •

edited

Loading

bdeggleston Jul 16, 2024

bdeggleston Jul 16, 2024

bdeggleston Jul 16, 2024

bdeggleston Jul 17, 2024

bdeggleston Jul 17, 2024

bdeggleston Jul 22, 2024

bdeggleston Jul 22, 2024

bdeggleston Jul 22, 2024

bdeggleston Jul 22, 2024

bdeggleston Jul 22, 2024

CASSANDRA-19744 Accord migration and interop correctness #3395

Are you sure you want to change the base?

CASSANDRA-19744 Accord migration and interop correctness #3395

Conversation

aweisberg commented Jul 2, 2024 • 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

aweisberg commented Jul 2, 2024 •

edited

Loading