CNDB-12425: A few reproduction tests and a preliminary patch, WIP #1529

ekaterinadimitrova2 · 2025-01-23T03:19:50Z

What is the issue

...
Queries with ALLOW FILTERING are rejected while we have an index that can be used in the future for the same query, still building

What does this PR fix and why was it fixed

CNDB-12425: A preliminary patch to facilitate discussions. It does not include:

feature flag
checks we are on the new messaging version added for ANNOptions
we fall back to allow filtering only on Index Creation. Currently, we also fall back to ALLOW FILTERING if we use nodetool to rebuild indexes

Checklist before you submit for review

Make sure there is a PR in the CNDB project updating the Converged Cassandra version
Use NoSpamLogger for log lines that may appear frequently in the logs
Verify test results on Butler
Test coverage for new/modified code is > 80%
Proper code formatting
Proper title for each commit staring with the project-issue number, like CNDB-1234
Each commit has a meaningful description
Each commit is not very long and contains related changes
Renames, moves and reformatting are in distinct commits

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

…t include: - feature flag - checks we are on the new messaging version added for ANNOptions - we fall back to allow filtering only on Index Creation. Currently we also fall back to ALLOW FILTERING if we use nodetool to rebuild indexes

sonarqubecloud · 2025-02-09T18:40:38Z

Quality Gate failed

Failed conditions
79.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

cassci-bot · 2025-02-09T18:44:27Z

✔️ Build ds-cassandra-pr-gate/PR-1529 approved by Butler

Approved by Butler
See build details here

ekaterinadimitrova2 · 2025-02-10T14:53:41Z

src/java/org/apache/cassandra/index/SecondaryIndexManager.java

@@ -382,6 +396,17 @@ public boolean isIndexQueryable(Index index)
        return isIndexQueryable(index.getIndexMetadata().name);
    }

+    /**
+     +     * Checks if the specified index is building.


Formatting issue, I will fix it

ekaterinadimitrova2 · 2025-02-10T15:06:03Z

test/unit/org/apache/cassandra/service/reads/range/RangeCommandsTest.java

@@ -77,7 +76,7 @@ public void tesConcurrencyFactor()
        // verify that a low concurrency factor is not capped by the max concurrency factor
        PartitionRangeReadCommand command = command(cfs, 50, 50);
        try (RangeCommandIterator partitions = RangeCommands.rangeCommandIterator(command, ONE, System.nanoTime(), ReadTracker.NOOP);
-             ReplicaPlanIterator ranges = new ReplicaPlanIterator(command.dataRange().keyRange(), command.indexQueryPlan(), keyspace, ONE))
+             ReplicaPlanIterator ranges = new ReplicaPlanIterator(command.dataRange().keyRange(), command.indexQueryPlan(), keyspace, ONE, false))


This should be command.rowFilter().allowFiltering

ekaterinadimitrova2 · 2025-02-10T17:49:31Z

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

-            searcher = indexSearcher();
-            Index index = indexQueryPlan.getFirst();
-            Tracing.trace("Executing read on {}.{} using index {}", cfs.metadata.keyspace, cfs.metadata.name, index.getIndexMetadata().name);
+            if (cfs.indexManager.isQueryableThroughIndex(indexQueryPlan, rowFilter().allowFiltering))


Collapsed the two ifs, no need of nesting here

adelapena · 2025-02-11T15:15:37Z

src/java/org/apache/cassandra/index/SecondaryIndexManager.java

@@ -362,13 +365,24 @@ public synchronized Future<?> addIndex(IndexMetadata indexDef, boolean isNewCF)
     * @param queryPlan a query plan
     * @throws IndexNotAvailableException if the query plan has any index that is not queryable
     */
-    public void checkQueryability(Index.QueryPlan queryPlan)
+    public boolean isQueryableThroughIndex(Index.QueryPlan queryPlan, boolean allowsFiltering)


This method can now return true or false, or throw an exception, and it also throws a client warning. That looks like too many side effects for a method starting with boolean is..., which might suggest a simpler behaviour. I would either:
a) Split it into two simpler separate boolean methods to know if all the indexes in the plan are building/queryable, and let ReadCommand#executeLocally do the AF check and throw exceptions and warnings.
b) Transform it into a SecondaryIndexManger#searcherFor(Index.QueryPlan, boolean) method keeping most of it's responsibilities, returning the searcher if it's possible to build it, null if it's building, and exception if it's not queryable.

adelapena · 2025-02-11T15:16:20Z

src/java/org/apache/cassandra/index/SecondaryIndexManager.java

+     +     */
+    public boolean isIndexBuilding(Index index)
+    {
+            return isIndexBuilding(index.getIndexMetadata().name);


Nit: double tab

adelapena · 2025-02-11T15:17:27Z

src/java/org/apache/cassandra/index/SecondaryIndexManager.java

+     +     * @param index the index
+     +     * @return <code>true</code> if the specified index is building, <code>false</code> otherwise
+     +     */
+    public boolean isIndexBuilding(Index index)


Nit: can we put the two overloads of isIndexBuilding together?

adelapena · 2025-02-11T15:19:45Z

src/java/org/apache/cassandra/index/SecondaryIndexManager.java

@@ -1814,13 +1839,16 @@ public static void shutdownAndWait(long timeout, TimeUnit units) throws Interrup
     * @param indexQueryPlan index query plan used in the read command
     * @param level consistency level of read command
     */
-    public static <E extends Endpoints<E>> E filterForQuery(E liveEndpoints, Keyspace keyspace, Index.QueryPlan indexQueryPlan, ConsistencyLevel level)
+    public static <E extends Endpoints<E>> E filterForQuery(E liveEndpoints, Keyspace keyspace, Index.QueryPlan indexQueryPlan, ConsistencyLevel level, boolean allowsFiltering)


Nit: break the long line and add the new allowsFiltering param to the method's JavaDoc.

adelapena · 2025-02-11T15:21:41Z

src/java/org/apache/cassandra/locator/ReplicaPlans.java

@@ -723,11 +723,11 @@ public static ReplicaPlan.ForRangeRead forSingleReplicaRead(Keyspace keyspace, A
     * The candidate collection can be used for speculation, although at present
     * it would break EACH_QUORUM to do so without further filtering
     */
-    public static ReplicaPlan.ForTokenRead forRead(Keyspace keyspace, Token token, Index.QueryPlan indexQueryPlan, ConsistencyLevel consistencyLevel, SpeculativeRetryPolicy retry)
+    public static ReplicaPlan.ForTokenRead forRead(Keyspace keyspace, Token token, Index.QueryPlan indexQueryPlan, ConsistencyLevel consistencyLevel, SpeculativeRetryPolicy retry, boolean allowsFiltering)


Nit: break the long line.

adelapena · 2025-02-11T15:22:28Z

src/java/org/apache/cassandra/locator/ReplicaPlans.java

@@ -743,11 +743,11 @@ public static ReplicaPlan.ForTokenRead forRead(Keyspace keyspace, Token token, I
     *
     * There is no speculation for range read queries at present, so we never 'always speculate' here, and a failed response fails the query.
     */
-    public static ReplicaPlan.ForRangeRead forRangeRead(Keyspace keyspace, Index.QueryPlan indexQueryPlan, ConsistencyLevel consistencyLevel, AbstractBounds<PartitionPosition> range, int vnodeCount)
+    public static ReplicaPlan.ForRangeRead forRangeRead(Keyspace keyspace, Index.QueryPlan indexQueryPlan, ConsistencyLevel consistencyLevel, AbstractBounds<PartitionPosition> range, int vnodeCount, boolean allowsFiltering)


Nit: break the long line.

adelapena · 2025-02-11T15:27:29Z

src/java/org/apache/cassandra/db/filter/RowFilter.java


    private final FilterElement root;

-    protected RowFilter(FilterElement root)
+    public final boolean allowFiltering;


We have a mixed use of allowFiltering and allowsFiltering around. I would homogenize it, so it's easier to search.

ALLOW FILTERING has always been kind of a transversal concept while it was a CQL thing. Putting it in the RowFilter somehow feels like it finally has a clear home. I think it would be super nice to add some brief JavaDoc here saying what ALLOW FILTERING is, for the sake of newcomers.

adelapena · 2025-02-11T15:33:16Z

src/java/org/apache/cassandra/db/filter/RowFilter.java

-    protected RowFilter(FilterElement root)
+    public final boolean allowFiltering;
+
+    protected RowFilter(FilterElement root, boolean allowFitlering)


Typo: s/allowFitlering/allowFiltering

adelapena · 2025-02-11T15:35:38Z

src/java/org/apache/cassandra/db/filter/RowFilter.java

@@ -1836,19 +1841,22 @@ public void serialize(RowFilter filter, DataOutputPlus out, int version) throws
        {
            out.writeBoolean(false); // Old "is for thrift" boolean
            FilterElement.serializer.serialize(filter.root, out, version);
+            out.writeBoolean(filter.allowFiltering);


This should depend on the messaging version

adelapena · 2025-02-11T15:35:48Z

src/java/org/apache/cassandra/db/filter/RowFilter.java

        }

        public RowFilter deserialize(DataInputPlus in, int version, TableMetadata metadata) throws IOException
        {
            in.readBoolean(); // Unused
            FilterElement operation = FilterElement.serializer.deserialize(in, version, metadata);
-            return new RowFilter(operation);
+            boolean allowFiltering = in.readBoolean();


This should depend on the messaging version

adelapena · 2025-02-11T15:37:53Z

src/java/org/apache/cassandra/db/filter/RowFilter.java

        }

        public long serializedSize(RowFilter filter, int version)
        {
            return 1 // unused boolean
-                   + FilterElement.serializer.serializedSize(filter.root, version);
+                   + FilterElement.serializer.serializedSize(filter.root, version)
+                   + 1; // for allowFiltering


This should depend on the messaging version. Also, it's probably better to use TypeSizes.BOOL_SIZE

adelapena · 2025-02-13T16:09:50Z

src/java/org/apache/cassandra/index/sai/StorageAttachedIndexGroup.java

+        }
+
+        // Verify that all provided indexes belong to this group
+        assert indexes.size() < indices.size() && indices.containsAll(saiIndexes);


I think we can simply add this to the loop above:

assert indices.contains(index) : "Index does not belong to this group";

adelapena · 2025-02-13T17:03:00Z

src/java/org/apache/cassandra/index/Index.java

+         * @return a new query plan for the specified {@link RowFilter} and {@link Index}, {@code null} otherwise
+         */
+        @Nullable
+        QueryPlan queryPlanForIndices(RowFilter rowFilter, Set<Index> indexes);


I'm still thinking about how could make this the only method and get rid of queryPlanFor(RowFilter)

adelapena · 2025-02-13T17:07:11Z

src/java/org/apache/cassandra/index/SingletonIndexGroup.java

@@ -120,6 +120,25 @@ public Index.QueryPlan queryPlanFor(RowFilter rowFilter)
        return null;
    }

+    @Override
+    public Index.QueryPlan queryPlanForIndices(RowFilter rowFilter, Set<Index> indexes)


This one looks like almost a duplicate of Index.QueryPlan queryPlanFor, which kinds of reinforces the idea that we could have a single signature for this.

This one is wrong in my opinion. This class is about single index, we can never build plan of a subset which would be 0 indexes... I have it throwing unsupported in my next changes. But I agree we may want to combine into one method, though it may be a bit confusing and overloaded.
As I mentioned - the current state is draft with main functionality for feedback - the actual polishing is in the making :-)

ekaterinadimitrova2 marked this pull request as draft January 23, 2025 03:20

ekaterinadimitrova2 self-assigned this Jan 23, 2025

ekaterinadimitrova2 commented Jan 23, 2025

View reviewed changes

src/java/org/apache/cassandra/db/ReadCommand.java Show resolved Hide resolved

ekaterinadimitrova2 force-pushed the CNDB-12425 branch from 9b73538 to e03373a Compare February 9, 2025 17:51

ekaterinadimitrova2 marked this pull request as ready for review February 9, 2025 17:53

ekaterinadimitrova2 marked this pull request as draft February 9, 2025 17:53

ekaterinadimitrova2 marked this pull request as ready for review February 9, 2025 17:55

Empty-Commit

9ed9b61

ekaterinadimitrova2 marked this pull request as draft February 9, 2025 17:57

ekaterinadimitrova2 marked this pull request as ready for review February 9, 2025 18:01

ekaterinadimitrova2 marked this pull request as draft February 9, 2025 21:10

Make testAllowFilteringDuringIndexBuildsOn3NodeCluster deterministic

7138378

ekaterinadimitrova2 commented Feb 10, 2025

View reviewed changes

adelapena reviewed Feb 11, 2025

View reviewed changes

ekaterinadimitrova2 requested a review from michaeljmarshall February 11, 2025 18:53

adelapena reviewed Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNDB-12425: A few reproduction tests and a preliminary patch, WIP #1529

CNDB-12425: A few reproduction tests and a preliminary patch, WIP #1529

ekaterinadimitrova2 commented Jan 23, 2025 •

edited

Loading

sonarqubecloud bot commented Feb 9, 2025

cassci-bot commented Feb 9, 2025

ekaterinadimitrova2 Feb 10, 2025

ekaterinadimitrova2 Feb 10, 2025

ekaterinadimitrova2 Feb 10, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 11, 2025

adelapena Feb 13, 2025

adelapena Feb 13, 2025

adelapena Feb 13, 2025

ekaterinadimitrova2 Feb 13, 2025

CNDB-12425: A few reproduction tests and a preliminary patch, WIP #1529

Are you sure you want to change the base?

CNDB-12425: A few reproduction tests and a preliminary patch, WIP #1529

Conversation

ekaterinadimitrova2 commented Jan 23, 2025 • edited Loading

What is the issue

What does this PR fix and why was it fixed

Checklist before you submit for review

sonarqubecloud bot commented Feb 9, 2025

Quality Gate failed

cassci-bot commented Feb 9, 2025

✔️ Build ds-cassandra-pr-gate/PR-1529 approved by Butler

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

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 commented Jan 23, 2025 •

edited

Loading