-
Notifications
You must be signed in to change notification settings - Fork 133
Add a default value for serial consistency: LocalSerial #291
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
Conversation
scylla/src/frame/types.rs
Outdated
pub fn validate_serial(cl: &Option<Self>) { | ||
match cl { | ||
None | Some(Consistency::Serial) | Some(Consistency::LocalSerial) => (), | ||
_ => panic!("Serial consistency can only be set to Serial or LocalSerial"), |
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.
By the way, I wonder if we couldn't split the Consistency
type into a regular Consistency
+ SerialConsistency
- there would be no need for runtime validation and less confusion for the users. Does it make sense to set Serial
or LocalSerial
for regular consistency?
On the other hand other drivers always seem to use the same consistency level type both for regular and serial consistency, maybe there is a good reason for that...
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.
That would indeed be best, but that's also why I mentioned maintaining backward compatibility - existing code could break for some users. That's why I went with a runtime check.
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.
Sigh, on the other hand, we're still in 0.x, so we can make breaking changes, and it makes so much sense to split the types... they are absolutely mutually exclusive, as it's also not legal to set a LOCAL_SERIAL level to the regular consistency level. I'll rework this pull request to make this breaking change and send it for review.
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.
There's also no point in following other drivers in that matter - if anything, our solution would just be more comprehensible to the users. It's ultimately CQL's fault that these levels were standardized in such a weird manner.
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.
@kostja Could you comment on #291 (comment) ? The rust driver has a type which represents the CQL's [consistency]
, but would like to split it into two - one which has SERIAL and LOCAL_SERIAL consistency levels, and one which has the others. Is there a good reason not to do that? I don't think any other driver which I know does that, but I'm not sure if there is a good reason behind that.
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.
I would be wonderful if there are two distinct types.
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.
I think go driver went that path
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.
SERIAL and LOCAL-SERIAL should be part of Consistency data type, but it's best to make SerialConsistency a distinct type to avoid programming bugs.
scylla/src/statement/batch.rs
Outdated
@@ -53,6 +53,7 @@ impl Batch { | |||
/// Sets the serial consistency to be used when executing this batch. | |||
/// (Ignored unless the batch is an LWT) | |||
pub fn set_serial_consistency(&mut self, sc: Option<Consistency>) { |
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.
If we decide not to split the Consistency
type, it would be nice if the set_serial_consistency
functions would have a comment which warns that the function will panic if the consistency level is invalid.
ffd22ab
to
787f880
Compare
v2:
|
This breaking change splits the Consistency enum into two: - Consistency, used for all queries - SerialConsistency, an additional flag used only for lightweight transactions The reason for the split is that these values are mutually exclusive: - it's only legal to use serial consistency for LWT, and it's not possible to use it in any other scope. Respectively, it's not possible to use any of the regular consistency values for defining serial consistency.
... to improve user experience. Serial consistency level can be reset to stronger, cross-DC consistency by calling set_serial_consistency explicitly.
787f880
to
c1e8293
Compare
@piodul rebased, review ping |
There's some clash with a recent bump of docs version, I'll debug. Meanwhile, the code itself is still reviewable |
The entry explains that lightweight transaction queries are almost like regular queries, except they have a serial consistency level in addition to the regular consistency level.
c1e8293
to
70a4cd1
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.
LGTM
This series adds a default serial consistency level for queries -
LocalSerial
. The decision is based on the fact thatLocalSerial
is the one that provides acceptable performance. Users interested in strong, cross-dc consistency - which could also cause prohibitive latencies - should override toSerial
.The series also comes with a doc entry which briefly explains LWT queries (they weren't mentioned at all so far), as well as an extra validation layer which would prevent users from setting a wrong consistency level, without introducing any backward incompatible changes.
This series introduces a breaking change - serial consistency is now defined as a separate enum. Fixing the existing aplications is trivial and consists of replacing all occurrences of
Consistency::LocalSerial
toSerialConsistency::LocalSerial
andConsistency::Serial
toSerialConsistency::Serial
.Fixes #277
Pre-review checklist
Fixes:
annotations to PR description.