CASSANDRA-19428 #3194

ekaterinadimitrova2 · 2024-03-21T17:58:44Z

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

* replace KeyRangeConcatIterator's PriorityQeueu with List * remove KeyRangeIterator.current and simplify * remove injected exception and tests - not relevant to the current implementation * expand randomized testing * inline getCurrent() -> peek(); rename getCount to getMaxKeys * redefine skipTo contract to not return a value (which saves unnecessary work when skipTo is called multiple times in a row) * calling hasNext in skipTo is a pessimization; if the iterator is in DONE state, then skipTo will see it and avoid further effort; if it is not, then we are computing a next value that we're just going to throw away * Fix a bug in QueryController#getIndexResults error handling

ekaterinadimitrova2 · 2024-03-22T13:53:53Z

Suggestion to change SingleNodeQueryFailureTest (passes locally for me)

/*
 * Licensed to the Apache Software Foundation (ASF) under one
 * or more contributor license agreements.  See the NOTICE file
 * distributed with this work for additional information
 * regarding copyright ownership.  The ASF licenses this file
 * to you under the Apache License, Version 2.0 (the
 * "License"); you may not use this file except in compliance
 * with the License.  You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package org.apache.cassandra.index.sai.disk;

import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;

import com.datastax.driver.core.exceptions.ReadFailureException;
import org.apache.cassandra.index.sai.SAITester;
import org.apache.cassandra.index.sai.disk.v1.postings.PostingListRangeIterator;
import org.apache.cassandra.index.sai.disk.v1.segment.LiteralIndexSegmentTermsReader;
import org.apache.cassandra.inject.Injection;
import org.apache.cassandra.inject.Injections;
import org.apache.cassandra.utils.Throwables;

import static org.apache.cassandra.inject.ActionBuilder.newActionBuilder;
import static org.apache.cassandra.inject.Expression.quote;
import static org.apache.cassandra.inject.InvokePointBuilder.newInvokePoint;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

public class SingleNodeQueryFailureTest extends SAITester
{
    private static final String CREATE_TABLE_TEMPLATE = "CREATE TABLE %s (id text PRIMARY KEY, v1 int, v2 text) WITH " +
            "compaction = {'class' : 'SizeTieredCompactionStrategy', 'enabled' : false }";

    @Before
    public void setup()
    {
        requireNetwork();
    }

    @After
    public void teardown()
    {
        Injections.deleteAll();
    }

    @Test
    public void testFailedRangeIteratorOnMultiIndexesQuery() throws Throwable
    {
        testFailedMultiIndexesQuery("range_iterator", PostingListRangeIterator.class, "getNextRowId");
    }

    @Test
    public void testFailedTermsReaderOnMultiIndexesQuery() throws Throwable
    {
        testFailedMultiIndexesQuery("terms_reader", LiteralIndexSegmentTermsReader.TermQuery.class, "lookupPostingsOffset");
    }

    private void testFailedMultiIndexesQuery(String name, Class<?> targetClass, String targetMethod) throws Throwable
    {
        Injection injection = Injections.newCustom(name)
                .add(newInvokePoint().onClass(targetClass).onMethod(targetMethod))
                .add(newActionBuilder().actions().doThrow(RuntimeException.class, quote("Injected failure!")))
                .build();

        createTable(CREATE_TABLE_TEMPLATE);
        createIndex(String.format(CREATE_INDEX_TEMPLATE, "v1"));
        createIndex(String.format(CREATE_INDEX_TEMPLATE, "v2"));

        execute("INSERT INTO %s (id, v1, v2) VALUES ('1', 0, '0')");
        flush();
        execute("INSERT INTO %s (id, v1, v2) VALUES ('2', 1, '1')");
        flush();
        execute("INSERT INTO %s (id, v1, v2) VALUES ('3', 2, '2')");
        flush();

        try
        {
            Injections.inject(injection);

            assertThatThrownBy(() -> executeNet("SELECT id FROM %s WHERE v1 < 1 and v2 = '0'"))
                    .isInstanceOf(ReadFailureException.class);

            assertThatThrownBy(() -> executeNet("SELECT id FROM %s WHERE v1 >= 1 and v2 = '1'"))
                    .isInstanceOf(ReadFailureException.class);

            assertThatThrownBy(() -> executeNet("SELECT id FROM %s WHERE v1 >= 2 and v2 = '2'"))
                    .isInstanceOf(ReadFailureException.class);
        }
        catch (Exception e)
        {
            throw Throwables.unchecked(e);
        }
        finally
        {
            injection.disable();
        }

        Assert.assertEquals(3, executeNet("SELECT id FROM %s WHERE v1 >= 0").all().size());
        Assert.assertEquals(1, executeNet("SELECT id FROM %s WHERE v2 = '0'").all().size());
        Assert.assertEquals(1, executeNet("SELECT id FROM %s WHERE v2 = '1'").all().size());
        Assert.assertEquals(1, executeNet("SELECT id FROM %s WHERE v2 = '2'").all().size());
    }
}

maedhroz · 2024-03-26T23:52:05Z

test/unit/org/apache/cassandra/index/sai/utils/SAIRandomizedTester.java

+    {
+        return getRandom().nextDouble();
+    }
+


Looking at these three methods, it seems like nextDouble() and randomDouble() produce the same result, since 0 + (1 - 0) * getRandom().nextDouble() == 1 * getRandom().nextDouble() == getRandom().nextDouble() right?

Totally! Let's leave nextDouble in and remove randomDouble ?

…troller

maedhroz · 2024-03-27T20:57:47Z

src/java/org/apache/cassandra/index/sai/plan/QueryController.java

            // all sstable indexes in view have been referenced, need to clean up when exception is thrown
            builder.cleanup();
-            queryView.referencedIndexes.forEach(SSTableIndex::releaseQuietly);
+            if (rangeCount == 0)
+                queryView.referencedIndexes.forEach(SSTableIndex::releaseQuietly);


See https://github.com/maedhroz/cassandra/tree/19428-5.0-3-release-indexes

maedhroz · 2024-03-27T21:04:18Z

src/java/org/apache/cassandra/index/sai/iterators/KeyRangeConcatIterator.java

+            throw new IllegalArgumentException("Cannot concatenate empty list of ranges");
+
+        this.ranges = ranges.iterator();
+        this.currentRange = this.ranges.next();
        this.toRelease = new ArrayList<>(ranges);


After looking at this, I think it might be worth trying to use the original approach I proposed in CASSANDRA-18165. We should be able to avoid creating a new iterator, and while we're here, I'm not really sure why we defensive copy ranges for toRelease when we've already created a new list in the builder. (If we used my implementation, we could rename toRelease to just ranges I guess?)

I know getCurrent() is gone, so it can't be a direct port, but I think we can still do this without all the extra object creation...

maedhroz · 2024-03-28T16:27:59Z

On SingleNodeQueryFailureTest, I think we agreed to test both single and multi-index query failures.

…removal; less object creation

…ter#nextDouble

maedhroz · 2024-03-28T23:56:18Z

test/unit/org/apache/cassandra/index/sai/utils/SAIRandomizedTester.java

@@ -129,6 +129,16 @@ public static int between(int min, int max)
        return getRandom().nextIntBetween(min, max - 1);
    }

+    public static double nextDouble()
+    {
+        return randomDoubleBetween(0, 1);


nit: I think this could also just be getRandom().nextDouble()?

randomDoubleBetween calls getRandom().nextDouble() under the hood. It seems more clear to me to use the randomDoubleBetween with the max and min explicitly stated. Though, now as you mentioned, I noticed that nextBoolean uses directly getRandom().nextBoolean(). I don't have strong preference for one or the other. I can change it

Yeah, it's equivalent...just seemed less complicated to go directly to getRandom().nextDouble()

maedhroz · 2024-03-28T23:59:21Z

src/java/org/apache/cassandra/index/sai/iterators/KeyRangeConcatIterator.java

-        this.ranges = ranges;
-        this.toRelease = new ArrayList<>(ranges);
+        this.current = 0;
+        this.ranges = new ArrayList<>(ranges);


nit: Do we even need the defensive copy? ranges is private and never accessible by users of the iterator, and there is already an ArrayList created in the builder, which is also private, right? The only way I could imagine we get in trouble is if someone adds to the builds after calling buildIterator(), but that's definitely incorrect usage/we should never do it :)

I guess I did not understand correctly this comment:

If we used my implementation, we could rename toRelease to just ranges I guess?

So you suggest it to be this.ranges = ranges? (I just checked that this is what we already do in other iterators too)

So you suggest it to be this.ranges = ranges?

Yeah, basically...the defensive copy just seems paranoid.

maedhroz

Left a couple tiny nits, but LGTM!

ekaterinadimitrova2 · 2024-04-03T23:18:42Z

The final approved patch is:
https://github.com/ekaterinadimitrova2/cassandra/tree/19428-5.0-5
https://github.com/ekaterinadimitrova2/cassandra/pull/new/19428-trunk-2
They were squashed and CI was successfully run:
5.0, fully green: https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra?branch=C19428-5.0-final
trunk, still in progress: https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra?branch=C19428-trunk-final
The patch was manually merged, closing the PR

ekaterinadimitrova2 added 2 commits March 21, 2024 12:53

DO NOT COMMIT

875d5be

Change SingleNodeQueryFailureTest

36e2667

maedhroz reviewed Mar 26, 2024

View reviewed changes

Rationalize/standardize the way we release SSTableIndexes in QueryCon…

a0b978c

…troller

maedhroz reviewed Mar 27, 2024

View reviewed changes

ekaterinadimitrova2 added 3 commits March 28, 2024 12:35

Use the proposed solution from CASSANDRA-18165 for the PriorityQueue …

b3d7d06

…removal; less object creation

Remove SAIRandomizedTester#randomeDouble in favor of SAIRandomizedTes…

4fc1bc0

…ter#nextDouble

Test single and multi index in SingleNodeQueryFailureTest

32331dc

maedhroz reviewed Mar 28, 2024

View reviewed changes

maedhroz approved these changes Mar 29, 2024

View reviewed changes

review comments

a09846d

ekaterinadimitrova2 closed this Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSANDRA-19428 #3194

CASSANDRA-19428 #3194

ekaterinadimitrova2 commented Mar 21, 2024

ekaterinadimitrova2 commented Mar 22, 2024

maedhroz Mar 26, 2024

ekaterinadimitrova2 Mar 27, 2024

maedhroz Mar 27, 2024

ekaterinadimitrova2 Mar 28, 2024

maedhroz Mar 27, 2024

ekaterinadimitrova2 Mar 28, 2024

maedhroz commented Mar 28, 2024

maedhroz Mar 28, 2024

ekaterinadimitrova2 Mar 29, 2024 •

edited

Loading

maedhroz Mar 29, 2024

ekaterinadimitrova2 Mar 29, 2024

maedhroz Mar 28, 2024 •

edited

Loading

ekaterinadimitrova2 Mar 29, 2024 •

edited

Loading

maedhroz Mar 29, 2024

ekaterinadimitrova2 Mar 29, 2024

maedhroz left a comment

ekaterinadimitrova2 commented Apr 3, 2024

CASSANDRA-19428 #3194

CASSANDRA-19428 #3194

Conversation

ekaterinadimitrova2 commented Mar 21, 2024

ekaterinadimitrova2 commented Mar 22, 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

maedhroz commented Mar 28, 2024

Choose a reason for hiding this comment

ekaterinadimitrova2 Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maedhroz Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

ekaterinadimitrova2 Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maedhroz left a comment

Choose a reason for hiding this comment

ekaterinadimitrova2 commented Apr 3, 2024

ekaterinadimitrova2 Mar 29, 2024 •

edited

Loading

maedhroz Mar 28, 2024 •

edited

Loading

ekaterinadimitrova2 Mar 29, 2024 •

edited

Loading