Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CNDB-12620: Add new reason RequestFailureReason.INDEX_BUILD_IN_PROGRESS #1537

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ekaterinadimitrova2
Copy link

@ekaterinadimitrova2 ekaterinadimitrova2 commented Jan 28, 2025

The second commit - cherry-picked one, should not be committed! It is just to help us build the current patch on top of the other work which is going to be committed soon.

What is the issue

...
Users should get INDEX_BUILD_IN_PROGRESS instead of INDEX NOT AVAILABLE when they cannot query because an index is building.

What does this PR fix and why was it fixed

...
We add a new reason, RequestFailureReason.INDEX_BUILD_IN_PROGRESS to differentiate indexes that are currently building in error messages

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

@ekaterinadimitrova2 ekaterinadimitrova2 marked this pull request as draft January 28, 2025 23:01
…codes according to Appache and avoid future conflicts

by assigning new codes to higher numbers
CNDB-7343: Fix fromCode method and add a unit test
CNDB-7343: Update forException method for IndexNotAvailable
CNDB-7343: Make forException method more resiliant against partial updates
@ekaterinadimitrova2 ekaterinadimitrova2 marked this pull request as ready for review January 29, 2025 19:20
@cassci-bot
Copy link

❌ Build ds-cassandra-pr-gate/PR-1537 rejected by Butler


1 new test failure(s) in 2 builds
See build details here


Found 1 new test failures

Test Explanation Branch history Upstream history
o.a.c.u.b.BinLogTest.testTruncationReleasesLogS... regression 🔴🔴 🔵🔵🔵🔵🔵🔵🔵

Found 9 known test failures

@ekaterinadimitrova2
Copy link
Author

org.apache.cassandra.utils.binlog.BinLogTest.testTruncationReleasesLogSpace - known flaky for a very long time - https://github.com/riptano/cndb/issues/10308. I guess it was just not failing lately in CI. Also, it did not fail locally for me and I do not see how it can be related to what we do here.
This seems to be ready for review. We need it pulled into Stargate RequestFailureReason too. I will open a PR soon and test it also in CNDB. Similar to what we did in #7343

Comment on lines +538 to +540
assertThatThrownBy(() -> execute("SELECT * FROM %s WHERE v=0 ALLOW FILTERING"))
.hasMessage("The secondary index '" + idx + "' is not yet available as it is building")
.isInstanceOf(IndexBuildInProgressException.class);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why try to use the index and fail instead of use filtering if the user allowed it?
I thought if I set AF in the query, the query should always run regardless of index availability.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AF issue is a different ticket where we are addressing this - #12425. Until that one is solved, we add as an intermediate this one as per agreement with interested parties.

Copy link

@pkolaczk pkolaczk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have one loosely related question that doesn't block this PR.

@ekaterinadimitrova2
Copy link
Author

@pkolaczk, thank you for. the review. Your question is valid and it will be addressed more thoroughly in a different ticket - #12425.
This is intermediately added as per request, until the AF issue is solved and thoroughly tested. There are a lot of corner cases. Ping me if you are interested to know more.

I will not commit this one yet, until the rest of the PRs in the original issue are in a good shape. More on that in the GH issue. We will also need Stargate release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants