CASSANDRA-19042: Repair fuzz tests fail with paxos_variant: v2 #3100

dcapwell · 2024-02-13T22:23:32Z

No description provided.

dcapwell · 2024-02-13T22:26:07Z

src/java/org/apache/cassandra/service/paxos/Paxos.java

@@ -385,13 +386,18 @@ public EndpointsForToken readCandidates()
        }

        static Participants get(TableMetadata table, Token token, ConsistencyLevel consistencyForConsensus)


I left this here as Paxos uses it... I updated the stuff that mattered for the existing tests to use the new API.

I did see PaxosRepair touches this as well, but I assume it didn't matter as we don't have data in paxos so that logic prob didn't get touched?

Weird, I don't know why my IDE Intellij Idea emits a warning that the Config class is deprecated. Not after your patch, though; it's just observation.

dcapwell · 2024-02-13T22:27:23Z

src/java/org/apache/cassandra/service/paxos/PaxosRepair.java

    {
-        EndpointState epState = Gossiper.instance.getEndpointStateForEndpoint(peer);


this is duplicate logic from Gossiper, the only difference was

try { return value.value; } catch (IllegalArgumentException e) { return null; }

Not sure how a field access leads to IllegalArgumentException, so removed in favor of the gossip function

I don't know about the IllegalArgumentException, probably some leftover

dcapwell · 2024-02-13T22:29:46Z

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosCleanupSession.java


 public class PaxosCleanupSession extends AsyncFuture<Void> implements Runnable,
                                                                      IEndpointStateChangeSubscriber,
                                                                      IFailureDetectionEventListener,
                                                                      RequestCallbackWithFailure<Void>
 {
-    private static final Map<UUID, PaxosCleanupSession> sessions = new ConcurrentHashMap<>();


moved to org.apache.cassandra.service.paxos.cleanup.PaxosRepairState

dcapwell · 2024-02-13T22:39:05Z

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosCleanupSession.java

@@ -95,49 +93,40 @@ private static ScheduledFuture<?> schedule(PaxosCleanupSession session)
        }
    }

-    private final UUID session = UUID.randomUUID();
+    private final SharedContext ctx;
+    public final UUID session = UUID.randomUUID();


this is non-deterministic, but this problem exists in repair as well... Repair uses random/time uuid outside of the random source/clock from SharedContext... since these are keys for a map, I have mostly looked the other way... it makes debugging annoying (as you can not block on the repair id) but its not been an issue so just ignore

dcapwell · 2024-02-13T22:41:03Z

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosFinishPrepareCleanup.java

@@ -77,93 +68,10 @@ public synchronized void onResponse(Message<Void> msg)
            trySuccess(null);
    }

-    static class PendingCleanup extends IntrusiveStack<PendingCleanup>


moved to org.apache.cassandra.service.paxos.cleanup.PaxosRepairState

dcapwell · 2024-02-13T22:42:20Z

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosRepairState.java

+
+    public void setSession(PaxosCleanupSession session)
+    {
+        //TODO (review): I copy/pasted from PaxosCleanupSession... this is a check-then-act pattern which is not thread safe...


I can fix if desired, I left alone so I could lower the risk in the refactor

Incremental changes are ok. I guess it is also in 4.1 so not a regression? Let's open a ticket for the community so someone can pick it up

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosRepairState.java

dcapwell · 2024-02-13T22:46:52Z

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosTableRepairs.java

@@ -211,25 +208,4 @@ AbstractPaxosRepair createRepair(DecoratedKey key, Ballot incompleteBallot, Cons
    {
        return PaxosRepair.create(consistency, key, incompleteBallot, table);
    }
-
-    private static final ConcurrentMap<TableId, PaxosTableRepairs> tableRepairsMap = new ConcurrentHashMap<>();


moved to org.apache.cassandra.service.paxos.cleanup.PaxosRepairState

dcapwell · 2024-02-13T22:49:39Z

test/unit/org/apache/cassandra/repair/FuzzTestBase.java

@@ -339,6 +346,12 @@ public Set<Faults> apply(Cluster.Node node, Message<?> message)
                    // these messages are not resilent to ephemeral issues
                    case STATUS_REQ:
                    case STATUS_RSP:
+                    // paxos repair does not support faults and will cause a TIMEOUT error, failing the repair


I tested each verb and we lead to timeout in different code paths... some times the user is told its paxos related, some times not... logs are always clear though at least...

This patch does not try to make these messages resilient to dropped messages or concurrent retries, so opts-out so these messages will be happy path only.

dcapwell · 2024-02-13T22:50:39Z

test/unit/org/apache/cassandra/repair/FuzzTestBase.java

@@ -654,6 +667,7 @@ static class Cluster

            // We run tests in an isolated JVM per class, so not cleaing up is safe... but if that assumption ever changes, will need to cleanup
            Stage.ANTI_ENTROPY.unsafeSetExecutor(orderedExecutor);
+            Stage.MISC.unsafeSetExecutor(orderedExecutor);


one line of code did this rather than Stage.ANTI_ENTROPY... not sure why we do MISC rather than ANTI_ENTROPY given this is repair... but figured best not to change it... For this test they are the same executor so doesn't matter.

dcapwell · 2024-02-13T22:51:56Z

test/unit/org/apache/cassandra/repair/FuzzTestBase.java

@@ -776,10 +790,46 @@ public void onFailure(InetAddressAndPort from, RequestFailureReason failureReaso
            }
        }

+        private static class CallbackKey


repair doesn't reuse messages/callbacks so didn't see that this logic was slightly different than MessagingService... so need to have both (message_id, peer) as the key and not just message_id, else the paxos messages clear the callback and hang forever (since it doesn't enqueue work the test actually fails right away).

test/unit/org/apache/cassandra/repair/FuzzTestBase.java

bdeggleston

+1

ekaterinadimitrova2

Overall LGTM (though limited knowledge in that part of the codebase, I admit). Some nits left which can be addressed on commit

ekaterinadimitrova2 · 2024-03-21T20:44:35Z

src/java/org/apache/cassandra/gms/IGossiper.java

+    void notifyFailureDetector(Map<InetAddressAndPort, EndpointState> remoteEpStateMap);
+    void applyStateLocally(Map<InetAddressAndPort, EndpointState> epStateMap);


These two can benefit from some javadoc

ekaterinadimitrova2 · 2024-03-21T21:22:28Z

src/java/org/apache/cassandra/net/MessageDelivery.java

@@ -28,4 +29,8 @@ public interface MessageDelivery
    public <REQ, RSP> void sendWithCallback(Message<REQ> message, InetAddressAndPort to, RequestCallback<RSP> cb, ConnectionType specifyConnection);
    public <REQ, RSP> Future<Message<RSP>> sendWithResult(Message<REQ> message, InetAddressAndPort to);
    public <V> void respond(V response, Message<?> message);
+    public default void respondWithFailure(RequestFailureReason reason, Message<?> message)


Can you remove the public modifiers from this interface, please?

to be consistent with the rest of the file, I left public there...

ekaterinadimitrova2 · 2024-03-21T22:14:13Z

src/java/org/apache/cassandra/service/paxos/Paxos.java

@@ -385,13 +386,18 @@ public EndpointsForToken readCandidates()
        }

        static Participants get(TableMetadata table, Token token, ConsistencyLevel consistencyForConsensus)


Weird, I don't know why my IDE Intellij Idea emits a warning that the Config class is deprecated. Not after your patch, though; it's just observation.

ekaterinadimitrova2 · 2024-03-22T16:25:22Z

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosRepairState.java

+
+    public void setSession(PaxosCleanupSession session)
+    {
+        //TODO (review): I copy/pasted from PaxosCleanupSession... this is a check-then-act pattern which is not thread safe...


Incremental changes are ok. I guess it is also in 4.1 so not a regression? Let's open a ticket for the community so someone can pick it up

ekaterinadimitrova2 · 2024-03-22T16:26:59Z

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosRepairState.java

+import static org.apache.cassandra.exceptions.RequestFailureReason.UNKNOWN;
+import static org.apache.cassandra.net.NoPayload.noPayload;
+
+public class PaxosRepairState


Please add Javadoc

Tracks the state of paxos repair cleanup work

ekaterinadimitrova2 · 2024-03-22T23:26:57Z

src/java/org/apache/cassandra/service/paxos/PaxosRepair.java

    {
-        EndpointState epState = Gossiper.instance.getEndpointStateForEndpoint(peer);


I don't know about the IllegalArgumentException, probably some leftover

test/unit/org/apache/cassandra/repair/FuzzTestBase.java

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosRepairState.java

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosCleanupSession.java

dcapwell commented Feb 13, 2024

View reviewed changes

src/java/org/apache/cassandra/service/paxos/cleanup/PaxosRepairState.java Show resolved Hide resolved

dcapwell commented Feb 13, 2024

View reviewed changes

test/unit/org/apache/cassandra/repair/FuzzTestBase.java Show resolved Hide resolved

dcapwell force-pushed the CASSANDRA-19042 branch from 8fa16d5 to 1d3abd9 Compare March 22, 2024 21:40

bdeggleston approved these changes Mar 25, 2024

View reviewed changes

dcapwell force-pushed the CASSANDRA-19042 branch from 1d3abd9 to fe9b7e2 Compare March 25, 2024 17:48

ekaterinadimitrova2 approved these changes Mar 25, 2024

View reviewed changes

CASSANDRA-19042: Repair fuzz tests fail with paxos_variant: v2

8c906d9

dcapwell force-pushed the CASSANDRA-19042 branch from 8cc5229 to 8c906d9 Compare March 25, 2024 21:37

smiklosovic closed this Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSANDRA-19042: Repair fuzz tests fail with paxos_variant: v2 #3100

CASSANDRA-19042: Repair fuzz tests fail with paxos_variant: v2 #3100

dcapwell commented Feb 13, 2024

dcapwell Feb 13, 2024

ekaterinadimitrova2 Mar 21, 2024

dcapwell Feb 13, 2024

ekaterinadimitrova2 Mar 22, 2024

dcapwell Feb 13, 2024

dcapwell Feb 13, 2024

dcapwell Feb 13, 2024

dcapwell Feb 13, 2024

ekaterinadimitrova2 Mar 22, 2024 •

edited

dcapwell Feb 13, 2024

dcapwell Feb 13, 2024

dcapwell Feb 13, 2024

dcapwell Feb 13, 2024

bdeggleston left a comment

ekaterinadimitrova2 left a comment

ekaterinadimitrova2 Mar 21, 2024

ekaterinadimitrova2 Mar 21, 2024

dcapwell Mar 25, 2024

ekaterinadimitrova2 Mar 21, 2024

ekaterinadimitrova2 Mar 22, 2024 •

edited

ekaterinadimitrova2 Mar 22, 2024

dcapwell Mar 25, 2024

ekaterinadimitrova2 Mar 22, 2024

		@@ -385,13 +386,18 @@ public EndpointsForToken readCandidates()
		}

		static Participants get(TableMetadata table, Token token, ConsistencyLevel consistencyForConsensus)

		{
		EndpointState epState = Gossiper.instance.getEndpointStateForEndpoint(peer);

		void notifyFailureDetector(Map<InetAddressAndPort, EndpointState> remoteEpStateMap);
		void applyStateLocally(Map<InetAddressAndPort, EndpointState> epStateMap);

CASSANDRA-19042: Repair fuzz tests fail with paxos_variant: v2 #3100

CASSANDRA-19042: Repair fuzz tests fail with paxos_variant: v2 #3100

Conversation

dcapwell commented Feb 13, 2024

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

ekaterinadimitrova2 Mar 22, 2024 • edited

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

bdeggleston left a comment

Choose a reason for hiding this comment

ekaterinadimitrova2 left a 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

ekaterinadimitrova2 Mar 22, 2024 • edited

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

ekaterinadimitrova2 Mar 22, 2024 •

edited

ekaterinadimitrova2 Mar 22, 2024 •

edited