[
https://issues.apache.org/jira/browse/CASSANDRA-18521?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17722231#comment-17722231
]
Andres de la Peña commented on CASSANDRA-18521:
-----------------------------------------------
Here is a patch:
||PR||CI||
|[cep-7-sai|https://github.com/apache/cassandra/pull/2329]|[j8|https://app.circleci.com/pipelines/github/adelapena/cassandra/2892/workflows/d6f09762-97f1-409f-90b7-aa69c7951668]
[j11|https://app.circleci.com/pipelines/github/adelapena/cassandra/2892/workflows/837b186e-6cd9-442c-baaf-8bc067f56b21]|
[The first
commit|https://github.com/apache/cassandra/pull/2329/commits/9722461016ab0a4ee4a0a0e55b5ed6bdd9ba7023]
moves {{SAITester#waitForIndexQueryable}} to {{CQLTester}} and renames the
methods. So {{CQLTester}} has separate {{waitForIndexQueryable}} and
{{waitForTableIndexesQueryable}} methods.
[The second
commit|https://github.com/apache/cassandra/pull/2329/commits/fbf68d158f6d074dc1cc2330397d45f5f4b936d5]
addresses the addition of a version of {{CQLTester#createIndex}} that waits
for the index to become queryable.
There are hundred of calls to {{createIndex}} outside the index package. Most
of these calls don't use any of the utility methods to wait for the index.
Instead, the rely on the fact that [legacy indexes don't have an initialization
task when the indexed table is
empty|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/index/internal/CassandraIndex.java#L171].
This makes the creation of a legacy index on an empty table a blocking
operation. Most tests exploit this by creating the index before inserting data,
so they don't have to wait for the index build. I think this comes from a time
when the methods for waiting for the index build didn't exist.
I think that approach is risky because it depends on the particular
implementation of secondary indexes. The index API doesn't offer any guarantees
of synchronicity when creating indexes on empty tables. Indeed, SAI always does
an asynchronous index creation. So those hundred tests can become flaky if
someday we make SAI the default implementation. Thus, I think that those tests
should use the variant of {{createIndex}} that waits for the index.
I think most new tests would also want to use the synchronous
{{{}createIndex{}}}. Developers unaware of the asynchronous index creation
should probably be directed to the synchronous {{{}createIndex{}}}.
On the other hand, the asynchronous {{createIndex}} seems to be useful for
tests validating the asynchronous building itself, and anyone writing those
tests would probably be familiarized index building mechanism they are testing.
So I have made {{CQLTester#createIndex}} the synchronous method, which should
solve the problem of not properly waiting for the index on the many tests
outside the index package. The asynchronous index creation can be done with a
new {{CQLTester#createIndexAsync}} method, which is equivalent to the previous
old {{{}createIndex{}}}, and that is only used for tests related with the index
building process.
It's worth mentioning that using the sync method when one needs to use the sync
method usually leads to a consistent assertion error when the wait for the
index build expires. However, using the async method when one needs to use the
sync method produces flaky tests that are much harder to catch. That's why I
prefer to make the sync method the default and not naming it
{{{}createIndexSync{}}}, {{{}createIndexWaitingForQueryable{}}}, or something
like that. wdyt?
I have also included some fixes in {{{}CQLTester#getCreateIndexName{}}}, which
was producing wrong index names in some cases.
> Unify CQLTester#waitForIndex and SAITester#waitForIndexQueryable
> ----------------------------------------------------------------
>
> Key: CASSANDRA-18521
> URL: https://issues.apache.org/jira/browse/CASSANDRA-18521
> Project: Cassandra
> Issue Type: Improvement
> Components: Feature/SAI
> Reporter: Andres de la Peña
> Assignee: Andres de la Peña
> Priority: Normal
> Fix For: NA
>
>
> It has been
> [mentioned|https://github.com/maedhroz/cassandra/pull/11#discussion_r1190166765]
> in CASSANDRA-18217 that {{CQLTester#waitForIndex}} and
> {{SAITester#waitForIndexQueryable}} can be unified into a single method.
> The same discussion mentions that it's easy to forget to call those methods
> after calling {{CQLTester#createIndex}}. So that method can be renamed to
> {{createIndexAsync}}, and we can create a new method that creates the index
> and waits for it ({{createIndexSync}}, {{createIndexAndWaitForQueryable}}, or
> something like that).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
