-
Notifications
You must be signed in to change notification settings - Fork 13
prepared: Fix consistency bug #331
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
prepared: Fix consistency bug #331
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a severe bug where any prepared statement was erroneously set to a default consistency of One, instead of leaving it as UNKNOWN to use the configured execution profile.
- Removed the line that set consistency to One in the prepared statement function.
- Removed the now-unused import of Consistency from scylla::frame::types::Consistency in session.rs, and updated test imports accordingly.
Comments suppressed due to low confidence (2)
scylla-rust-wrapper/src/session.rs:777
- Consider adding or updating tests in this module to verify that prepared statements inherit the correct default consistency (UNKNOWN) from the execution profile.
use scylla_cql::Consistency;
The fixed bug is quite severe: any prepared statement would have its consistency set to `One` by default. This is surprising, because `One` hasn't been the default consistency for prepared (or any other kind of) statements in CPP Driver since 2015 (as per the changelog)... Anyway, the CPP Driver's behaviour upon preparing is to set consistency in prepared statements to `UNKNOWN`, which means "use the consistency configured in the exec profile/cluster". This is the behaviour of Rust Driver, too: `PreparedStatement` has no consistency set by default. Thus, the fix is quite simple: we just remove the line that sets consistency to `One` in the `cass_session_prepare_n` function.
2b151b6
to
165d955
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"use the consistency configured in the exec profile/cluster".
What is the cpp-driver default for that? I see that the PR that introduced this set_consistency
line aimed to align the behavior with cpp-driver - so perhaps it just went about it the wrong way?
Per-cluster default consistencyThe default per-cluster consistency has changed in time.
So for many years it's been LocalOne. Per-exec profile default consistencyIt is unset ( Per-statement default consistencyIt is unset ( Our buggy implementationThe This is why I believe this is a severe bug. |
Do you have some links to commits / PRs of that? |
The bug
The found bug is quite severe: any prepared statement would have its consistency set to
One
by default. This is surprising, becauseOne
hasn't been the default consistency for prepared (or any other kind of) statements in CPP Driver since 2015 (as per the changelog)...The expected behaviour
Anyway, the CPP Driver's behaviour upon preparing is to set consistency in prepared statements to
UNKNOWN
, which means "use the consistency configured in the exec profile/cluster". This is the behaviour of Rust Driver, too:PreparedStatement
has no consistency set by default.The fix
Thus, the fix is quite simple: we just remove the line that sets consistency to
One
in thecass_session_prepare_n
function.Testing
Again, the IT for consistencies is being written. The test has already confirmed the bug's existence and the fix.
Pre-review checklist
[ ] I have enabled appropriate tests inMakefile
in{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER
.[ ] I added appropriateFixes:
annotations to PR description.