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

Option fix #4

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Claude-at-Instaclustr
Copy link

Changes for Cassandra processing.

Adds Other version type.
Returns an Error if the parsing fails
Refactored to make parsing easier to read.

@rukai
Copy link
Member

rukai commented Oct 12, 2021

I think you had a discussion with alex about maintainership, was he wanting to continue maintaining cassandra-proto?
If that is the case I imagine most of this should be upstreamed instead of acculumating into our fork?

@Claude-at-Instaclustr
Copy link
Author

I think you had a discussion with alex about maintainership, was he wanting to continue maintaining cassandra-proto? If that is the case I imagine most of this should be upstreamed instead of acculumating into our fork?

Yes, we should push upstream once we get everything working. This is a largish change to the protocol parser.

@rukai
Copy link
Member

rukai commented Oct 29, 2021

Any opportunity to upstream small changes that are correct in isolation should be taken.
It'll take a while for the maintainer to get around to reviewing and they also dont want to review huge chunks at once.

But I totally get that it can be hard to know what is correct until we get things fully working.

conorbros pushed a commit to conorbros/cassandra-proto that referenced this pull request Nov 3, 2021
zero-initialize buffer before passing to `Read`
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