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

Fix cassandra protocol null handling #784

Merged
merged 7 commits into from
Sep 15, 2022

Conversation

rukai
Copy link
Member

@rukai rukai commented Sep 2, 2022

Previously our implementation of the cassandra protocol was succesfully decoding null values but when it tried to reencode the null value it would output the bytes 00 00 00 04 FF FF FF FF instead of just FF FF FF FF.
This resulted in e.g. null ip addresses showing up as 255.255.255.255 and integers as -1.

Unfortunately solving this requires completely rewriting cassandra-protocols encoding implementation because of some poor fundamental decisions made there.
The problem is that the recursive generation (for e.g. List and Map types) relies on the Bytes various impl From<T> for Bytes.
However its impossible to represent a Null value on a Bytes, instead the Bytes needs to be wrapped in a CBytes in order to represent Null
This makes it impossible to handle both recursive types (list/map/set etc) and Null values using cassandra-protocol.

In order to get this working immediately for us I've just implemented the changes directly in shotover, its only a little more code shotover side but the custom implementation is:

  • faster due to drastically reducing allocations
  • easier to understand, all the logic is right in front of us rather than behind multiple layers of abstraction (IDE's have very poor support for navigating into into() implementations)
  • we can look to upstream it in the future, but getting it working within shotover is a good first step, it at least illustrates what a good solution should look like when we go to upstream it.

@rukai rukai force-pushed the fix_cassandra_protocol_null branch 5 times, most recently from 96e0eba to b0e4c99 Compare September 2, 2022 06:37
@shotover shotover deleted a comment from github-actions bot Sep 2, 2022
@rukai rukai force-pushed the fix_cassandra_protocol_null branch 6 times, most recently from 4d6cd37 to 9339179 Compare September 6, 2022 23:27
@rukai rukai force-pushed the fix_cassandra_protocol_null branch from 9339179 to 2700672 Compare September 8, 2022 06:55
@rukai rukai force-pushed the fix_cassandra_protocol_null branch from 2700672 to 386d93d Compare September 9, 2022 01:14
@rukai rukai force-pushed the fix_cassandra_protocol_null branch from 6987b46 to fbfd265 Compare September 9, 2022 05:26
@rukai rukai marked this pull request as ready for review September 9, 2022 06:14
@conorbros
Copy link
Member

Is it worth making a PR to cdrs-tokio or do you think just keeping it in Shotover is the best way to go?

I'd be happy to merge as is, but wondering how about future plans.

@rukai
Copy link
Member Author

rukai commented Sep 12, 2022

I absolutely want to investigate if we can upstream to cdrs-tokio as soon as we get the chance to, might be a while before we get that chance though.

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.

LGTM - I know it probably feels like a workaround at the moment given the underlying limitations, but happy to land this and work out pushing this upstream as mentioned in the comments above.

@rukai rukai enabled auto-merge (squash) September 15, 2022 04:23
@rukai rukai merged commit 2b938cc into shotover:main Sep 15, 2022
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