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

spec_exec: fix can_be_ignored function #1124

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

muzarski
Copy link
Contributor

ref: #1083
This is a first iteration of fixing #1083 - one we can merge before 0.15. I think this is crucial for speculative execution's performance - current implementation does not ignore any of the DbErrors. If any DbError is returned from database, we return prematurely without further investigation.

Previous implementation did not make much sense. We would prematurely return from speculative execution for some errors which could be ignored.

I adjusted the function, so we use #[deny(clippy::wildcard_enum_match_arm)] lint. It will force the developer to think before adjusting this function if new QueryError variant is introduced.

I categorized the errors (see the comments in the code) and justified why each of them needs to be ignored or not.

Note: In the comments, I mention that there are some error variants we don't expect there. It's true, because not all of the QueryError variants could be constructed at this stage. I'm currently working on narrowing the error type passed to this function (as well as errors passed to RetrySession::decide_should_retry and LBP::on_query_failure).

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

Previous implementation did not make much sense. We would prematurely return
from speculative execution for some errors which could be ignored.

I adjusted the function, so we use `#[deny(clippy::wildcard_enum_match_arm)]`
lint. It will force the developer to think before adjusting this function
if new QueryError variant is introduced.

I categorized the errors (see the comments in the code) and justified
why each of them needs to be ignored or not.
@muzarski muzarski self-assigned this Nov 13, 2024
@muzarski muzarski added this to the 0.15.0 milestone Nov 13, 2024
@muzarski muzarski added the bug Something isn't working label Nov 13, 2024
Copy link

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 5f1e007

@wprzytula wprzytula merged commit 8122c65 into scylladb:main Nov 13, 2024
11 checks passed
@wprzytula wprzytula mentioned this pull request Nov 14, 2024
@muzarski muzarski deleted the spec_exec_errors branch December 19, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants