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

Options rewrite #988

Closed
wants to merge 3 commits into from
Closed

Options rewrite #988

wants to merge 3 commits into from

Conversation

conorbros
Copy link
Member

@conorbros conorbros commented Jan 17, 2023

Shotover does not support compression in Cassandra yet. Changes in this PR:

  • Reject STARTUP messages if they attempt to use compression.
  • Add a transform for rewriting the SUPPORTED response to OPTIONS requests so users can disable compression

@conorbros conorbros marked this pull request as ready for review January 23, 2023 05:52
@rukai
Copy link
Member

rukai commented Jan 23, 2023

This PR contains two separate large new features, they should be landed in two separate PRs.

For the second feature I'm not quite sure what the use case is?
Like is there a reason the user would particularly care about what cassandra reports? Do drivers fallback if compression is reported as unsupported?

Also while a quick hack to make shotover handle its lack of compression support more professionally is welcome, if doing so ends up quite complicated we are better off just adding the compression support in the first place.

@conorbros
Copy link
Member Author

I'll split these into 2 PR's but address those comments here.

Here's an example of a driver checking the OPTIONS response before connecting.

https://github.com/scylladb/scylla-rust-driver/blob/ce9654473f713310290e42321d289daa6b3766ed/scylla/src/transport/connection.rs#L1210

Regardless it makes sense to not break the protocol, we should not be reporting compression supported if it's not. This is a temporary fix, the plan is to introduce full compression support.

@conorbros
Copy link
Member Author

Opened #989, #990, #991

@conorbros conorbros closed this Jan 24, 2023
@conorbros conorbros deleted the options-rewrite branch May 23, 2024 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants