Cassandra 20402 trunk #3955

ekaterinadimitrova2 · 2025-03-06T00:01:58Z

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

…ROGRESS

ekaterinadimitrova2 · 2025-03-06T00:06:59Z

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

@@ -316,7 +317,13 @@ public void checkQueryability(Index.QueryPlan queryPlan)
        for (Index index : queryPlan.getIndexes())
        {
            if (!isIndexQueryable(index))
+            {
+                // In Astra index can be queryable during index build, thus we need to check both not queryable and building
+                if (isIndexBuilding(index))


Hmmm, we probably need to use instead directly the full rebuild status here instead? Same as in filterForQuery

You're right. I don't think we'd want to count the cases where we aren't doing a full/initial build.

ekaterinadimitrova2 · 2025-03-06T00:12:28Z

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

+                            "SELECT pk FROM " + ks2 + '.' + cf1 + " WHERE v1=0 AND v2=0"
+                    ))
+                    .hasMessageContaining("Operation failed - received 1 responses and 1 failures: INDEX_BUILD_IN_PROGRESS");


In 2i ALLOW FILTERING seems to pass here and this query we have here instead complains of lack of ALLOW FILTERING and not about index not being available or building

Which makes sense considering:
https://github.com/apache/cassandra/pull/3955/files#r1990063343

src/java/org/apache/cassandra/exceptions/RequestFailureReason.java

maedhroz · 2025-03-11T17:51:45Z

src/java/org/apache/cassandra/exceptions/RequestFailureReason.java

+        exceptionToReasonMap.put(IndexBuildInProgressException.class, INDEX_BUILD_IN_PROGRESS);
+
+        if (exceptionToReasonMap.size() != reasons.length-3)
+            throw new RuntimeException("A new RequestFailureReasons was probably added and you may need to update the exceptionToReasonMap");


What if we just add a nullable field to the enum Class<? extends Throwable> exception and do...

if (reason.exception != null) exceptionToReasonMap.put(reason.exception, reason);

...in the loop above? Seems like it helps us avoid the need for this size check, which could get brittle.

Hmmm, that makes a lot of sense. I'm not sure why I didn't do it. Honestly, I can't think of a good reason. Thanks

maedhroz · 2025-03-11T17:59:51Z

src/java/org/apache/cassandra/exceptions/RequestFailureReason.java

-
+    // below code is intentionally higher, coming as a back port from a fork. It was created that high to prevent
+    // any conflicts with Apache Cassandra code.
+    INDEX_BUILD_IN_PROGRESS  (503);


I'm guessing this might not be the only one of these. Thinking of a general comment that takes that into account...maybe something like...

// The following codes have been ported from an external fork, where they were offset explicitly to avoid conflicts.

src/java/org/apache/cassandra/exceptions/RequestFailureReason.java

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

maedhroz · 2025-03-11T18:22:11Z

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

@@ -316,7 +317,13 @@ public void checkQueryability(Index.QueryPlan queryPlan)
        for (Index index : queryPlan.getIndexes())
        {
            if (!isIndexQueryable(index))
+            {
+                // In Astra index can be queryable during index build, thus we need to check both not queryable and building


nit: I guess we should eventually remove the specific reference to Astra?

Actually, it seems it applies to any non-SAI index implementation. Looking into isQueryable() (which is used to decide if we add to queryableIndexes) - it returns always true for non-SAI implementations.

Yeah, for some historical reason CassandraIndex looks like...

return status == Status.BUILD_SUCCEEDED || status == Status.UNKNOWN || status == Status.FULL_REBUILD_STARTED;

...whereas SAI won't say it's queryable during a full rebuild.

Yeah, my bad, CassandraIndex overrides it too since recently, but still, only BUILD_FAILED and DROPPED means non-queryable

So something like that? (with non-destroyed by GH formatting, of course)

public void checkQueryability(Index.QueryPlan queryPlan) { InetAddressAndPort endpoint = FBUtilities.getBroadcastAddressAndPort(); for (Index index : queryPlan.getIndexes()) { String indexName = index.getIndexMetadata().name; Index.Status indexStatus = getIndexStatus(endpoint, keyspace.getName(), indexName); if (!isIndexQueryable(index)) { // Plus isQueryable is always true for non-SAI index implementations, thus we need to check both not queryable and building if (indexStatus == Index.Status.FULL_REBUILD_STARTED) throw new IndexBuildInProgressException(index); throw new IndexNotAvailableException(index); } } }

Yeah, that looks reasonable

maedhroz · 2025-03-11T18:28:08Z

test/unit/org/apache/cassandra/index/sai/cql/AllowFilteringTest.java

@@ -18,13 +18,17 @@

 package org.apache.cassandra.index.sai.cql;

+import org.apache.cassandra.index.IndexBuildInProgressException;
+import org.apache.cassandra.inject.Injections;
+import org.apache.cassandra.inject.InvokePointBuilder;


nit: move these C* imports down

test/unit/org/apache/cassandra/index/sai/cql/AllowFilteringTest.java

maedhroz · 2025-03-11T18:33:00Z

test/unit/org/apache/cassandra/index/sai/cql/AllowFilteringTest.java

+        String idx = createIndexAsync(String.format("CREATE CUSTOM INDEX ON %%s(v) USING '%s'", StorageAttachedIndex.class.getName()));
+
+        assertThatThrownBy(() -> execute("SELECT * FROM %s WHERE v=0"))
+                .hasMessage("The secondary index '" + idx + "' is not yet available as it is building")


I wonder if we could make a little constant out of the message string/pattern in IndexBuildInProgressException? That way we could reuse it here (and in some other tests?) to build error messages without worrying if we'll change them later (assuming they still take just the index name).

I have it and I forgot to commit it :-) Good call

maedhroz · 2025-03-11T18:36:24Z

test/unit/org/apache/cassandra/exceptions/RequestFailureReasonTest.java

+        assertEquals(RequestFailureReason.NOT_CMS, RequestFailureReason.fromCode(8));
+        assertEquals(RequestFailureReason.INVALID_ROUTING, RequestFailureReason.fromCode(9));
+        assertEquals(RequestFailureReason.COORDINATOR_BEHIND, RequestFailureReason.fromCode(10));
+        assertEquals(RequestFailureReason.INDEX_BUILD_IN_PROGRESS, RequestFailureReason.fromCode(503));


nit: Can we just use EXPECTED_VALUES to test these in a loop? At least that way we only have to modify EXPECTED_VALUES when we add a new code...

maedhroz · 2025-03-11T18:37:01Z

test/unit/org/apache/cassandra/cql3/validation/entities/SecondaryIndexTest.java

@@ -25,6 +25,8 @@
 import java.util.concurrent.CountDownLatch;

 import com.google.common.collect.ImmutableSet;
+
+import org.apache.cassandra.index.IndexBuildInProgressException;


nit: move import down

maedhroz · 2025-03-11T18:37:20Z

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

@@ -27,6 +27,7 @@
 import java.util.concurrent.TimeUnit;

 import com.google.common.base.Objects;
+import org.apache.cassandra.distributed.api.ConsistencyLevel;


nit: move import down

maedhroz · 2025-03-11T18:37:53Z

test/unit/org/apache/cassandra/index/sai/cql/AllowFilteringTest.java

@@ -391,4 +395,30 @@ private void test(String query, boolean requiresAllowFiltering) throws Throwable
        assertNotNull(execute(query + " ALLOW FILTERING"));
    }

+    private final Injections.Barrier blockIndexBuild = Injections.newBarrier("block_index_build", 2, false)
+                                                                 .add(InvokePointBuilder.newInvokePoint().onClass(StorageAttachedIndex.class)
+                    .onMethod("startInitialBuild"))


nit: formatting?

ekaterinadimitrova2 added 2 commits March 5, 2025 18:45

CASSANDRA-20402: Add new reason RequestFailureReason.INDEX_BUILD_IN_P…

699b3aa

…ROGRESS

DO NOT COMMIT CircleCI config for testing

2e4e769

ekaterinadimitrova2 requested review from maedhroz and jasonstack March 6, 2025 00:02

ekaterinadimitrova2 commented Mar 6, 2025

View reviewed changes