Fix serial consistency handling #330

wprzytula · 2025-06-28T20:04:55Z

Generated with GPT-4o and manually (heavily) redacted

Problem overview

Serial consistency is handled incorrectly in both: 1) statement/batch, 2) exec profiles/cluster. The requirements that are not always met:

CASS_CONSISTENCY_ANY shall result in not setting any serial consistency in the request frame;
CASS_CONSISTENCY_UNKNOWN shall have the semantics of "ignore me and use the default". CPP Driver allows it for statements/batches/exec profiles and forbids for cluster (because there would be no default to be used over cluster-level configuration).

Solution

All changes are done to align with the CPP Driver behaviour.

The commits do the following:

Support for serial consistency ANY in statements and batches:
Fixed the semantics in statements and batches (cass_{statement,batch}_set_serial_consistency):
- ANY is allowed.
- UNKNOWN is allowed and means that the execution profile's setting is used (or cluster's, if there's no execution profile set). Unfortunately, as the Rust driver misses corresponding option to unset consistency on statements, this is unsupported for now.
(Bonus) Refactored consistency handling in statements and batches:
Similarly to what is done about serial consistency, the code is prepared for the Rust Driver's change introducing statements' API to unset consistency.
Fixes to serial consistency handling in execution profiles:
Fixed the semantics in execution profiles (cass_execution_profile_set_serial_consistency):
- ANY is allowed.
- UNKNOWN is allowed and means that the cluster's setting is used. This is unsupported for now and will be taken care in the follow-up.
Fixes to serial consistency handling in cluster:
- Cluster forbids CASS_CONSISTENCY_UNKNOWN.
Removal of SerialConsistency conversion:
- Eliminated the impl TryFrom<CassConsistency> for SerialConsistency implementation.
- Addressed misuse risks due to differing semantics in conversion contexts (statements/batches/execution profiles vs cluster).
No serial consistency as the default for the cluster:
- CPP Driver, as a default, sets no serial consistency.
- The default is adjusted to conform.
- Note: Rust driver purposefully set the default serial consistency to LocalSerial instead of Any. See the commit message for considerations. Let's discuss this.

Discussion needed

The last commit: should we change the CPP-Driver's default of serial consistency from Any to LocalSerial?

Summary:

These changes improve correctness of serial consistency handling, ensuring alignment with the CPP Driver's behavior.

Testing

Coming soon, I'm working on an integration test for consistency/serial consistency set in all possible ways. This is going to be in a follow-up.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
PR description sums up the changes and reasons why they should be introduced.
I have implemented Rust unit tests for the features/changes introduced.
~~[ ] I have enabled appropriate tests in Makefile in {SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.~~
~~[ ] I added appropriate Fixes: annotations to PR description.~~

Copilot

Pull Request Overview

This PR corrects serial consistency handling across statements, batches, execution profiles, and clusters to align with the C++ driver semantics.

Introduces get_serial_consistency_from_cass_consistency helper to map CassConsistency to an unset/set model.
Updates FFI setters in statement.rs and batch.rs to use the new helper and error on unsupported unsetting.
Refactors execution profile and cluster methods to delegate serial consistency logic to a shared builder helper and removes the old TryFrom for serial consistency.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
statement.rs	Added helper for mapping serial consistency, updated the FFI setter to use it.
exec_profile.rs	Added methods to set serial consistency via the helper and removed inline logic.
cluster.rs	Delegates cluster-level serial consistency setting to the exec profile builder helper.
cass_types.rs	Removed outdated `TryFrom<CassConsistency> for SerialConsistency` implementation.
batch.rs	Updated batch FFI setter to use the new helper and unified error semantics.

Comments suppressed due to low confidence (3)

scylla-rust-wrapper/src/statement.rs:691

[nitpick] Add a doc comment explaining how each CassConsistency variant maps to MaybeUnset<Option<SerialConsistency>>, and note the current limitation around unsetting serial consistency until the Rust driver adds support.

pub(crate) fn get_serial_consistency_from_cass_consistency(

scylla-rust-wrapper/src/statement.rs:691

Introduce unit tests for get_serial_consistency_from_cass_consistency covering UNKNOWN, ANY, LOCAL_SERIAL, and SERIAL, and add integration tests for the FFI setters to verify the new behavior.

pub(crate) fn get_serial_consistency_from_cass_consistency(

scylla-rust-wrapper/src/exec_profile.rs:17

The imported MaybeUnset from scylla::value may not match the crate’s MaybeUnset returned by get_serial_consistency_from_cass_consistency. Consider importing crate::value::MaybeUnset (or re-exporting the same type) to avoid a type mismatch at compile time.

use scylla::value::MaybeUnset;

Copilot · 2025-06-29T05:21:13Z

scylla-rust-wrapper/src/statement.rs

+    else {
+        return CassError::CASS_ERROR_LIB_BAD_PARAMS;
+    };
+    let serial_consistency = match maybe_set_serial_consistency {


[nitpick] The MaybeUnset::Unset error handling is duplicated in both statement.rs and batch.rs. You could extract this pattern into a shared helper or macro to reduce code duplication and make future changes easier.

Lorak-mmk · 2025-06-30T18:34:36Z

UNKNOWN is allowed and means that the execution profile's setting is used (or cluster's, if there's no execution profile set). Unfortunately, as the Rust driver misses corresponding option to unset consistency on statements, this is unsupported for now.

I don't understand this. First you say it is allowed, then you say it is unsupported. Which is it? What are the semantics in this PR?

Lorak-mmk · 2025-06-30T18:40:07Z

UNKNOWN is forbidden, because there's no source of the default setting over the execution profile.

Isn't a cluster such a source?

scylla-rust-wrapper/src/batch.rs

scylla-rust-wrapper/src/statement.rs

scylla-rust-wrapper/src/exec_profile.rs

wprzytula · 2025-07-01T05:42:37Z

UNKNOWN is allowed and means that the execution profile's setting is used (or cluster's, if there's no execution profile set). Unfortunately, as the Rust driver misses corresponding option to unset consistency on statements, this is unsupported for now.

I don't understand this. First you say it is allowed, then you say it is unsupported. Which is it? What are the semantics in this PR?

I'm sorry for this. Initially, I misunderstood semantics of UNKNOWN in execution profiles (I thought they forbid it similarly to the cluster). Later I fixed implementation and descriptions, but some were accidentally left stale. Now it should be consistent everywhere that execution profiles accept UNKNOWN.

The module `config_value` is introduced to handle configuration values that can be either set or unset. It features a trait `MaybeUnsetConfigValue` and an enum `MaybeUnsetConfig` that work in tandem to represent configuration values that may or may not be set. `MaybeUnsetConfigValue` is a Rust type that can be either set or unset, together with a constant defining the special "unset" C value and with methods to obtain the Rust type from a C value. For now, implementations of `MaybeUnsetConfigValue` are provided for Consistency and Option<SerialConsistency>, which are used in the next commits to handle (serial) consistency in a more robust way. In the future, more implementations can be added to this module as needed, with request timeout duration and retry policy being my first candidates. Intended usage will be shown soon, when those types are used in the configuration code.

CPP Driver allows setting `CASS_CONSISTENCY_UNKNOWN` on statement or batch to indicate that consistency is not set, which means "use whatever the execution profile/cluster specifies". Let's ensure consistent behaviour in CPP-Rust Driver. Unfortunately, the Rust Driver does not support "unsetting" consistency from a statement at the moment. For now, we will throw an error upon `CASS_CONSISTENCY_UNKNOWN` provided, in order to warn the user about this limitation. The change is done by leveraging the new `MaybeUnsetConfig` type to represent a consistency value that can be either set or unset.

CPP Driver allows setting serial consistency to `ANY`, which results in not setting serial consistency at all. Also, `CASS_CONSISTENCY_UNKNOWN` can be used on statement or batch to indicate that serial consistency is not set, which means "use whatever the execution profile/cluster specifies". Let's ensure consistent behaviour in CPP-Rust Driver. Unfortunately, the Rust Driver does not support "unsetting" serial consistency from a statement at the moment. For now, we will throw an error upon `CASS_CONSISTENCY_UNKNOWN` provided, in order to warn the user about this limitation. The change is done by leveraging the new `MaybeUnsetConfig` type to represent a serial consistency value that can be either set or unset.

CPP Driver allows setting `CASS_CONSISTENCY_UNKNOWN` on execution profile to indicate that consistency is not set, which means "use whatever the cluster specifies". Let's ensure consistent behaviour in CPP-Rust Driver. Note that I previously mistreated the `CASS_CONSISTENCY_UNKNOWN` value as an "any" value. This is incorrect: `CASS_CONSISTENCY_UNKNOWN` denotes "ignore me", which means that if for example a statement has `CASS_CONSISTENCY_UNKNOWN` set, then the execution profile should be considered instead. To compare, `CASS_CONSISTENCY_ANY` set on a statement overrides the execution profile. Actually, if a CPP execution profile specifies `CASS_CONSISTENCY_UNKNOWN` for its setting, then this setting is taken from the cluster = from the default execution profile (which forbids `CASS_CONSISTENCY_UNKNOWN`, which is ensured in the next commits). I'm going to introduce the described mechanism in a follow-up PR. For now, `CASS_CONSISTENCY_UNKNOWN` passed to `cass_execution_profile_set_consistency` results in an error `CASS_ERROR_LIB_BAD_PARAMS` returned. The change is done by leveraging the new `MaybeUnsetConfig` type to represent a consistency value that can be either set or unset.

Similarly to previous commits, this change ensures that the serial consistency is set to `None` when the value is `CASS_CONSISTENCY_ANY`. This aligns with the behavior of the CPP Driver. Note that I previously mistreated the `CASS_CONSISTENCY_UNKNOWN` value as an "any" value. This is incorrect: `CASS_CONSISTENCY_UNKNOWN` denotes "ignore me", which means that if for example a statement has `CASS_CONSISTENCY_UNKNOWN` set, then the execution profile should be considered instead. To compare, `CASS_CONSISTENCY_ANY` set on a statement overrides the execution profile. Actually, if a CPP execution profile specifies `CASS_CONSISTENCY_UNKNOWN` for its setting, then this setting is taken from the cluster = from the default execution profile (which forbids `CASS_CONSISTENCY_UNKNOWN`, which is ensured in the next commit). I'm going to introduce the described mechanism in a follow-up PR. For now, `CASS_CONSISTENCY_UNKNOWN` passed to `cass_execution_profile_set_serial_consistency` results in an error `CASS_ERROR_LIB_BAD_PARAMS` returned. The change is done by leveraging the new `MaybeUnsetConfig` type to represent a consistency value that can be either set or unset.

CPP Driver forbids setting `CASS_CONSISTENCY_UNKNOWN` on cluster to indicate that consistency is not set, because cluster must specify some default consistency to be used if there's no execution profile-level nor statement-level consistency specified. This behaviour has already been replicated in the Rust wrapper. This commit merely leverages the new `MaybeUnsetConfig` type to represent a consistency value that can be either set or unset, in order to unify the handling of unset consistency values and prevent bugs.

CPP Driver forbids setting `CASS_CONSISTENCY_UNKNOWN` on cluster to indicate that serial consistency is not set, because cluster must specify some default serial consistency to be used if there's no execution profile-level nor statement-level serial consistency specified. This behaviour has already been replicated in the Rust wrapper. This commit merely leverages the new `MaybeUnsetConfig` type to represent a serial consistency value that can be either set or unset, in order to unify the handling of unset serial consistency values and prevent bugs.

`impl TryFrom<CassConsistency> for (Serial)Consistency` are removed, as they are: - Not used anymore in the codebase. - Prone to misuse, because there are two different semantics of such conversion: 1. For statement/batch/exec_profile, where `CASS_CONSISTENCY_UNKNOWN` is allowed and must be handled. 2. For cluster, where `CASS_CONSISTENCY_UNKNOWN` is not allowed, and must result in an error. As a bug-resitant alternative, the `MaybeUnsetConfig` type is promoted.

The default serial consistency in the Rust driver is LocalSerial, which is different to Any in the CPP driver. This commit sets the default serial consistency to None, which is equivalent to Any. This makes CPP-Rust driver consistent with the CPP driver in terms of the default serial consistency. Note: Rust driver purposefully set the default serial consistency to LocalSerial instead of None. The rationale taken from the issue (scylladb/scylla-rust-driver#277): > Using lightweight transactions in CQL requires setting an additional > consistency level, the so called serial consistency level, on top of > the regular one. The serial consistency level can only take two > values: SERIAL and LOCAL_SERIAL. There's currently no default, so > using lwt results in an error message: `Consistency level for LWT is > missing for a request with conditions`. We should consider picking > a sensible default value in order to improve the user experience > of the driver. Considering this, we should consider changing the default serial consistency in CPP-Rust driver to LocalSerial, too. This commit finishes corrections made to consistency handling.

The relevant issue: scylladb/scylla-ccm#646 has been fixed some time ago.

scylla-rust-wrapper/src/cluster.rs

wprzytula added this to the 0.6 milestone Jun 28, 2025

wprzytula requested a review from Lorak-mmk June 28, 2025 20:04

wprzytula self-assigned this Jun 28, 2025

wprzytula added the bug Something isn't working label Jun 28, 2025

wprzytula requested a review from Copilot June 29, 2025 05:20

Copilot AI reviewed Jun 29, 2025

View reviewed changes

wprzytula force-pushed the fix-serial-consistency-handling branch from afbd50b to 57dd009 Compare June 29, 2025 17:34

wprzytula added the P1 P1 priority item - very important label Jun 29, 2025

wprzytula mentioned this pull request Jun 29, 2025

Fix driver defaults - cluster & exec profiles #332

Merged

4 tasks

wprzytula force-pushed the fix-serial-consistency-handling branch 2 times, most recently from 88c23e4 to b93e8b9 Compare June 30, 2025 16:46

wprzytula mentioned this pull request Jun 30, 2025

Expose public rust API & enable API lints #333

Merged

3 tasks

Lorak-mmk requested changes Jun 30, 2025

View reviewed changes

scylla-rust-wrapper/src/batch.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/statement.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/exec_profile.rs Outdated Show resolved Hide resolved

wprzytula force-pushed the fix-serial-consistency-handling branch from b93e8b9 to f9c986a Compare July 1, 2025 05:38

wprzytula force-pushed the fix-serial-consistency-handling branch 2 times, most recently from 6b763ba to a514860 Compare July 1, 2025 13:43

wprzytula added 10 commits July 1, 2025 15:54

Makefile: revert CCM_COMMIT_ID to master

f846baa

The relevant issue: scylladb/scylla-ccm#646 has been fixed some time ago.

wprzytula force-pushed the fix-serial-consistency-handling branch from a514860 to f846baa Compare July 1, 2025 13:54

Lorak-mmk approved these changes Jul 1, 2025

View reviewed changes

scylla-rust-wrapper/src/cluster.rs Show resolved Hide resolved

wprzytula merged commit c622d61 into scylladb:master Jul 5, 2025
11 checks passed

wprzytula deleted the fix-serial-consistency-handling branch July 5, 2025 08:20

wprzytula modified the milestones: 0.6, 0.5.1 Jul 6, 2025

wprzytula mentioned this pull request Jul 15, 2025

Release 0.5.1 #346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix serial consistency handling #330

Fix serial consistency handling #330

Uh oh!

wprzytula commented Jun 28, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jun 29, 2025

Uh oh!

Lorak-mmk commented Jun 30, 2025

Uh oh!

Lorak-mmk commented Jun 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wprzytula commented Jul 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix serial consistency handling #330

Fix serial consistency handling #330

Uh oh!

Conversation

wprzytula commented Jun 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem overview

Solution

Discussion needed

Summary:

Testing

Pre-review checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 29, 2025

Choose a reason for hiding this comment

Uh oh!

Lorak-mmk commented Jun 30, 2025

Uh oh!

Lorak-mmk commented Jun 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wprzytula commented Jul 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wprzytula commented Jun 28, 2025 •

edited

Loading