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.

Sign up for GitHub

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

Jump to bottom

CASSGO-26 consistency serial was added #1837

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

CASSGO-26 consistency serial was added #1837

wants to merge 1 commit into from
+125 −43

Conversation

OleksiienkoMykyta
Copy link

Closes #1832
With the previous implementation, setting consistency SERIAL for Paxos reads was impossible. Fixed in this PR

joao-r-reis
joao-r-reis reviewed Nov 5, 2024
View reviewed changes
Copy link
Contributor

@joao-r-reis joao-r-reis left a comment

Choose a reason for hiding this comment

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

Can't we just remove SerialConsistency altogether and use Consistency everywhere with these changes? I think it would make no difference and we'd get rid of a type that no longer has a reason to exist. Also we're missing a check on the ClusterConfig serial consistency to ensure it's a valid value.

On the other hand we could also keep SerialConsistency around to ensure that only SERIAL or LOCAL_SERIAL can be used when setting the serial consistency level. But to do this we couldn't make it type SerialConsistency = Consistency like it is on this PR. We'd have to keep the previous code for SerialConsistency AND add a Serial+LocalSerial for the Consistency type...

I believe that other drivers don't have a separate consistency level type for SERIAL CONSISTENCY so I'm +1 on the first option (i.e. remove SerialConsistency completely and just use Consistency everywhere with appropriate checks when setting the serial consistency).

@joao-r-reis joao-r-reis changed the title consistency serial was added CASSGO-26 consistency serial was added Nov 5, 2024
@OleksiienkoMykyta
Copy link
Author

OleksiienkoMykyta commented Nov 8, 2024 •
edited
Loading

@joao-r-reis I had an idea to implement it as you said in the first option, but it's the breaking changes, so I decided to keep backward compatibility. In case this feature is added to v2, there is no reason to leave SerialConsistency.
I added the check to make sure that consistency is valid.

@joao-r-reis
Copy link
Contributor

@joao-r-reis I had an idea to implement it as you said in the first option, but it's the breaking changes, so I decided to keep backward compatibility. In case this feature is added to v2, there is no reason to leave SerialConsistency. I added the check to make sure that consistency is valid.

Hmm what about if we have type SerialConsistency = Consistency like you did on this PR AND we replace all references of the SerialConsistency type (on parameters etc.) to Consistency? In this scenario I don't think there will be a breaking change. And we can mark SerialConsistency as deprecated

@joao-r-reis
Copy link
Contributor

So basically the only change we would add to this PR is make sure that SerialConsistency is not referenced anywhere (replacing with Consistency) and marking SerialConsistency as deprecated

joao-r-reis
joao-r-reis reviewed Nov 8, 2024
View reviewed changes
session.go Outdated
@@ -144,6 +144,10 @@ func NewSession(cfg ClusterConfig) (*Session, error) {
return nil, errors.New("Can't use both Authenticator and AuthProvider in cluster config.")
}

if cfg.Consistency.IsSerial() {
return nil, fmt.Errorf("the default consistency level is not allowed to be SERIAL or LOCAL_SERIAL. Recived value: %v", cfg.Consistency)
Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's fine if Consistency has SERIAL or LOCAL_SERIAL (it won't work with writes but sure, maybe the app only has reads). I meant that cfg.SerialConsistency has to be SERIAL or LOCAL_SERIAL

Copy link
Author

Choose a reason for hiding this comment

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

I agree, it slipped my mind that consistency could be SERIAL for read operation, thank you for noticing it.

@joao-r-reis
Copy link
Contributor

Looks good, please format your commit message following the rules stated in CONTRIBUTING.md and add this JIRA to the CHANGELOG.md in the Added section of Unreleased, thanks!

We need another committer +1 to merge this.

@OleksiienkoMykyta
Copy link
Author

@joao-r-reis I had an idea to implement it as you said in the first option, but it's the breaking changes, so I decided to keep backward compatibility. In case this feature is added to v2, there is no reason to leave SerialConsistency. I added the check to make sure that consistency is valid.

Hmm what about if we have type SerialConsistency = Consistency like you did on this PR AND we replace all references of the SerialConsistency type (on parameters etc.) to Consistency? In this scenario I don't think there will be a breaking change. And we can mark SerialConsistency as deprecated

I like this idea, it will solve the issue

@OleksiienkoMykyta OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch 3 times, most recently from accb87e to a18e16e Compare November 13, 2024 09:16
ribaraka
ribaraka reviewed Nov 13, 2024
View reviewed changes
frame.go Outdated
Comment on lines 206 to 208
EachQuorum Consistency = 0x07
LocalOne Consistency = 0x0A
Serial Consistency = 0x08
LocalSerial Consistency = 0x09
Copy link

@ribaraka ribaraka Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

NIT: move Serial and LocalSerial above LocalOne to maintain sequential order.

session.go
Comment on lines +147 to +149
if cfg.SerialConsistency > 0 && !cfg.SerialConsistency.IsSerial() {
return nil, fmt.Errorf("the default SerialConsistency level is not allowed to be anything else but SERIAL or LOCAL_SERIAL. Recived value: %v", cfg.SerialConsistency)
}
Copy link

Choose a reason for hiding this comment

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

How about adding a test for this?

Copy link

Choose a reason for hiding this comment

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

And overall, how about adding another test to verify that these two consistency levels are allowed for use?

Copy link
Author

Choose a reason for hiding this comment

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

I have added some tests to cover each consistency level, please check it

@OleksiienkoMykyta OleksiienkoMykyta force-pushed the issue-1832-add-consistency-serial branch 2 times, most recently from 57f784b to 769a5c1 Compare November 14, 2024 11:05
@OleksiienkoMykyta
CASSGO-26 consistency serial was added
e963b69 
The user should be able to set consistency to SERIAL or LOCAL_SERIAL
for Paxos reads, but the previous implementation doesn't support such a feature.

patch by Mykyta Oleksiienko; reviewed by João Reis for CASSGO-26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ribaraka ribaraka ribaraka approved these changes

@joao-r-reis joao-r-reis joao-r-reis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CASSGO-26 Impossible to set CONSISTENCY SERIAL (select consistency)
3 participants
@OleksiienkoMykyta @joao-r-reis @ribaraka