CASSANDRA-20639 Replica filtering protection can trigger short-read protection too aggressively when the LIMIT is less than the number of results in a partition #4150

maedhroz · 2025-05-09T06:37:11Z

No description provided.

maedhroz · 2025-05-13T03:39:23Z

src/java/org/apache/cassandra/utils/CloseableIterator.java

@@ -24,6 +24,8 @@
 public interface CloseableIterator<T> extends Iterator<T>, AutoCloseable
 {
    public void close();
+
+    public default void checkpoint() { }


This is very awkward, although it pokes the necessary hole in the contract for RFP to be able to do what it needs to do without closing the merged iterator and therefore the merge listener. Any other ideas?

instead of adding this to the CloseableIterator interface, why don't you make the anonymous MergeListener in ReplicaFilteringProtection a class that you can manipulate or "checkpoint" from queryProtectedPartitions?

This is done, and if preliminary testing is any indication, it's working reasonably well.

maedhroz · 2025-05-13T03:39:44Z

...uted/org/apache/cassandra/distributed/test/cql3/MultiNodeTableWalkWithoutReadRepairTest.java

-        // Example: builder.withSeed(42L);
+//        builder.withSeed(1210048824849624538L).withExamples(1);
+//        builder.withSeed(-7862021736520593557L).withExamples(1);
+//        builder.withSeed(602472426346856339L).withExamples(1);


seeds that failed variously during development

Will just revert these before commit...

maedhroz · 2025-05-13T03:41:28Z

test/distributed/org/apache/cassandra/distributed/test/sai/StrictFilteringTest.java

+
+        String select = withKeyspace("SELECT * FROM %s.short_read_no_srp WHERE k = 0 AND a = 1 LIMIT 1");
+        Iterator<Object[]> initialRows = CLUSTER.coordinator(1).executeWithPaging(select, ConsistencyLevel.ALL, 2);
+        assertRows(initialRows, row(0, 2, 1));


Imagine a partition with 10,000 rows, 100 of which are matches for an index or filtering query. With a limit of 10, the logic before this patch would have to keep issuing SRP reads to find all 100 matches, which kills performance...almost to a comical extent.

maedhroz · 2025-05-19T22:51:58Z

src/java/org/apache/cassandra/db/partitions/PartitionIterators.java

-                    partition.next();
-            }
-        }
-    }


Inlined this to RFP to have a complete view of everything happening in queryProtectedPartitions(). Open to reorganizing again once we're convinced things are correct.

bdeggleston · 2025-05-20T21:58:40Z

src/java/org/apache/cassandra/service/reads/ReplicaFilteringProtection.java

@@ -270,6 +277,11 @@ public void onMergedRangeTombstoneMarkers(RangeTombstoneMarker merged, RangeTomb

                    @Override
                    public void close()
+                    {
+                    }


should close call checkpoint also?

I think, as it stands, there's no situation where calling close() on the merge listener would do anything useful, given we're manually checkpointing in queryProtectedPartitions(), which used to close the iterator before this patch (hence the checkpoint logic being in close()). Looking at the other comment below about whether we even want to modify the iterator contracts, let's come back to this...

bdeggleston

+1 thanks

jasonstack · 2025-05-24T02:27:54Z

src/java/org/apache/cassandra/service/reads/ReplicaFilteringProtection.java

                if (partitions.isEmpty())
                {
-                    PartitionIterators.consumeNext(merged);
+                    if (consumeEntirePartitions)


is there a reason that non-null currentRowIterator is not closed here?

nvm, currentRowIterator is used when consumeEntirePartitions is false

jasonstack · 2025-05-24T07:56:55Z

test/distributed/org/apache/cassandra/distributed/test/sai/StrictFilteringTest.java

+
+        Long srpRequestsBefore = CLUSTER.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore("no_srp_at_limit").metric.shortReadProtectionRequests.getCount());
+
+        String select = withKeyspace("SELECT * FROM %s.no_srp_at_limit WHERE k = 0 AND a = 1 LIMIT 1");


is RFP triggered here? I assume yes, just in case "node1" has different a value for k=1 and c=3

Yes. Neither node has two complete rows for c = 2 and c = 3 in k = 0.

jasonstack

+1

test/distributed/org/apache/cassandra/distributed/test/sai/StrictFilteringTest.java

…t read protection reads patch by Caleb Rackliffe; reviewed by Blake Eggleston and Zhao Yang for CASSANDRA-20639

maedhroz · 2025-05-28T22:13:02Z

Committed as fe13726

maedhroz changed the title ~~very rough, likely not efficient WIP on CASSANDRA-20639~~ very rough WIP on CASSANDRA-20639 May 12, 2025

maedhroz force-pushed the CASSANDRA-20639-trunk branch 2 times, most recently from 530fd9c to 3058c0f Compare May 13, 2025 03:23

maedhroz changed the title ~~very rough WIP on CASSANDRA-20639~~ CASSANDRA-20639 Replica filtering protection can trigger short-read protection too aggressively when the LIMIT is less than the number of results in a partition May 13, 2025

maedhroz force-pushed the CASSANDRA-20639-trunk branch from 3058c0f to 5d60103 Compare May 13, 2025 03:34

maedhroz commented May 13, 2025

View reviewed changes

maedhroz force-pushed the CASSANDRA-20639-trunk branch 2 times, most recently from 50b8ae0 to 75fbd7a Compare May 13, 2025 05:33

maedhroz commented May 19, 2025

View reviewed changes

bdeggleston reviewed May 20, 2025

View reviewed changes

maedhroz force-pushed the CASSANDRA-20639-trunk branch 3 times, most recently from 170839a to 4c261cc Compare May 22, 2025 20:06

maedhroz requested a review from bdeggleston May 22, 2025 20:07

bdeggleston approved these changes May 23, 2025

View reviewed changes

jasonstack reviewed May 24, 2025

View reviewed changes

jasonstack approved these changes May 24, 2025

View reviewed changes

maedhroz force-pushed the CASSANDRA-20639-trunk branch 2 times, most recently from 4511780 to fc3d8d2 Compare May 27, 2025 21:59

maedhroz commented May 28, 2025

View reviewed changes

test/distributed/org/apache/cassandra/distributed/test/sai/StrictFilteringTest.java Outdated Show resolved Hide resolved

Ensure replica filtering protection does not trigger unnecessary shor…

aa6d77b

…t read protection reads patch by Caleb Rackliffe; reviewed by Blake Eggleston and Zhao Yang for CASSANDRA-20639

maedhroz force-pushed the CASSANDRA-20639-trunk branch from fc3d8d2 to aa6d77b Compare May 28, 2025 20:21

maedhroz closed this May 28, 2025


		Long srpRequestsBefore = CLUSTER.get(1).callOnInstance(() -> Keyspace.open(KEYSPACE).getColumnFamilyStore("no_srp_at_limit").metric.shortReadProtectionRequests.getCount());

		String select = withKeyspace("SELECT * FROM %s.no_srp_at_limit WHERE k = 0 AND a = 1 LIMIT 1");

CASSANDRA-20639 Replica filtering protection can trigger short-read protection too aggressively when the LIMIT is less than the number of results in a partition #4150

CASSANDRA-20639 Replica filtering protection can trigger short-read protection too aggressively when the LIMIT is less than the number of results in a partition #4150

Uh oh!

Conversation

maedhroz commented May 9, 2025

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!

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!

bdeggleston left a comment

Choose a reason for hiding this comment

Uh oh!

jasonstack May 24, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maedhroz May 27, 2025 • 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!

jasonstack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maedhroz commented May 28, 2025

Uh oh!

Uh oh!

jasonstack May 24, 2025 •

edited

Loading

maedhroz May 27, 2025 •

edited

Loading