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

Cassandra protocol v5 #1104

Merged
merged 14 commits into from
Apr 3, 2023
Merged

Cassandra protocol v5 #1104

merged 14 commits into from
Apr 3, 2023

Conversation

conorbros
Copy link
Member

@conorbros conorbros commented Mar 28, 2023

Will supersede the previous v5 PR. This PR implements support for the Cassandra v5 protocol without support for compression or non-self contained frames, which will be completed in followups.

@conorbros
Copy link
Member Author

conorbros commented Mar 28, 2023

Oops, I'll pull out that flamegraph change. -
#1105

@conorbros conorbros requested a review from rukai March 28, 2023 02:13
Copy link
Member

@rukai rukai left a comment

Choose a reason for hiding this comment

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

Heres some feedback so far, I'll continue the review tomorrow.

shotover-proxy/src/codec/cassandra.rs Outdated Show resolved Hide resolved
shotover-proxy/src/codec/cassandra.rs Outdated Show resolved Hide resolved
shotover-proxy/src/codec/cassandra.rs Outdated Show resolved Hide resolved
shotover-proxy/src/codec/cassandra.rs Outdated Show resolved Hide resolved
shotover-proxy/src/codec/cassandra.rs Outdated Show resolved Hide resolved
shotover-proxy/src/codec/cassandra.rs Outdated Show resolved Hide resolved
shotover-proxy/src/codec/cassandra.rs Outdated Show resolved Hide resolved
@rukai
Copy link
Member

rukai commented Mar 29, 2023

I've realized that none of our benchmarks are actually running in protocol v5:

  • our criterion benches use casandra-cpp which uses v4 by default (but supposedly supports v5)
  • our latte benches uses scylla driver under the hood which only supports protocol v4

We will need to get access to some benchmarks before we can land this as modern drivers will enable v5 by default.

A way avoid this unbenchmarked protocol support from hitting production while still landing this PR would be setting a feature flag (that is enabled in tests) such that we reject any attempts to connect with v5 for now.

@conorbros conorbros force-pushed the protocol-v5-refactor branch 3 times, most recently from fcb0470 to cd40ae8 Compare March 30, 2023 03:02
Copy link
Member

@rukai rukai left a comment

Choose a reason for hiding this comment

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

Thankyou for all your efforts in getting this through.
Lets land this!

Copy link
Member

@benbromhead benbromhead left a comment

Choose a reason for hiding this comment

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

Good work on this one!

@rukai rukai enabled auto-merge (squash) April 3, 2023 04:01
@rukai rukai merged commit 9128d66 into shotover:main Apr 3, 2023
@conorbros conorbros deleted the protocol-v5-refactor branch April 3, 2023 08:23
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.

3 participants