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

CASSGO-1 CASSGO-30 Native Protocol 5 Support #1822

Open
wants to merge 26 commits into
base: trunk
Choose a base branch
from

Conversation

worryg0d
Copy link

@worryg0d worryg0d commented Sep 16, 2024

Overview

C* 4.0 introduced Native Protocol 5, so we should make gocql to support it.
This PR consists of other my PRs, which also provides support for the v5 version of the protocol.

This PR provides support of Native Protocol 5 features as:

@worryg0d worryg0d force-pushed the native-proto-5-support-all-merged branch 4 times, most recently from d67fcb1 to 93933c6 Compare September 19, 2024 12:49
@worryg0d worryg0d force-pushed the native-proto-5-support-all-merged branch 3 times, most recently from 47a3ae9 to b617638 Compare October 1, 2024 07:03
@joao-r-reis joao-r-reis added the protocol_v5 Tickets related to supporting protocol version 5 label Oct 1, 2024
@worryg0d
Copy link
Author

worryg0d commented Oct 1, 2024

This PR is merged from #1778, #1773, #1782, and #1781, so we could close them in favor of this one

@joao-r-reis
Copy link
Contributor

What about #1783

Copy link
Member

@lukasz-antoniak lukasz-antoniak left a comment

Choose a reason for hiding this comment

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

Copied unsolved comments fromCopied unresolved comments from previous PRs.

frame_test.go Show resolved Hide resolved
@lukasz-antoniak
Copy link
Member

What about #1783

@worryg0d, IMHO changes in #1783 are still needed and improve code maintainability.

@worryg0d
Copy link
Author

worryg0d commented Oct 9, 2024

Thanks a lot for reaching out!

This PR is lacking changes from #1783. I will ask @testisnullus if we could merge it into this PR.

@testisnullus
Copy link

@worryg0d @lukasz-antoniak I think we can keep the current implementation and merge the #1783 PR into this one.

@worryg0d worryg0d force-pushed the native-proto-5-support-all-merged branch from b617638 to 3f50a90 Compare October 10, 2024 12:06
@worryg0d worryg0d force-pushed the native-proto-5-support-all-merged branch from 3f50a90 to fbdaa2b Compare October 10, 2024 12:14
@worryg0d
Copy link
Author

I merged changes from #1783 into this one.

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.

First pass review. I'll need some extra time to properly review the envelope/message format changes.

batch_test.go Outdated Show resolved Hide resolved
cassandra_test.go Show resolved Hide resolved
compressor.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
session.go Outdated Show resolved Hide resolved
frame.go Show resolved Hide resolved
frame.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
@worryg0d
Copy link
Author

Hey, @joao-r-reis. Thanks a lot for your feedback! I'm pleased this PR is moving. I will take a look at your comments and suggestions tomorrow. I hope everything will be fine here.

@worryg0d
Copy link
Author

Hello, @joao-r-reis. I added some changes following your comments and suggestions. Could you please take a look at this one again? Also, I have a question regarding to cache of prepared statements. Could you please look at this one too?

Thanks a lot.

conn.go Show resolved Hide resolved
@worryg0d worryg0d force-pushed the native-proto-5-support-all-merged branch from 2b7d196 to 2a5056d Compare October 28, 2024 09:52
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.

Almost there! I think the functionality is now done and everything looks to be working correctly 👍 These last few comments are small things.

Still need another committer to take a look at this PR though.

conn.go Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
conn.go Outdated Show resolved Hide resolved
// 2.3.1 Initial Handshake
// In order to support both v5 and earlier formats, the v5 framing format is not
// applied to message exchanges before an initial handshake is completed.
startupCompleted := &atomic.Bool{}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to keep this one open until we have a second reviewer to look at this.

conn.go Show resolved Hide resolved
@joao-r-reis
Copy link
Contributor

Added semver-major label due to the Compressor interface changes

2. The driver should relly on No_metadata flag instead of Skip_metadata when handling RESULT/ROWS responses from C*
…_changed handling logic by forcing the driver to execute queries with old metadata

2. Added checking of integrity of the result to the test
@worryg0d worryg0d force-pushed the native-proto-5-support-all-merged branch 2 times, most recently from 6370ae0 to b5d76bd Compare October 30, 2024 16:58
…rent code base uses initial keyspace provided by the user to construct the keys. Since proto v5 we also should account for keyspace bounding for a specific query, so the driver should use the bounded keyspace instead of the initial to construct the key.

2. Changed the way how routing key cache keys are constructed to account the keyspace overriding as well.
@worryg0d worryg0d force-pushed the native-proto-5-support-all-merged branch from b5d76bd to 0298a00 Compare October 31, 2024 14:56
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.

Good stuff 👍 , I left 2 conversations open so that we get input from the other committer that looks at this:

  • bumping go.mod version to 1.19 so we can use atomic.Bool (current minimum version is 1.17 but go.mod is not up to date
  • Compressor interface changes

I'm +1 on both of these though.

@joao-r-reis
Copy link
Contributor

oh I just noticed the integration tests on GH are running with snappy compression, we should change it to lz4 so we get coverage on compression+v5

@worryg0d
Copy link
Author

Good stuff 👍 , I left 2 conversations open so that we get input from the other committer that looks at this:

  • bumping go.mod version to 1.19 so we can use atomic.Bool (current minimum version is 1.17 but go.mod is not up to date
  • Compressor interface changes

I'm +1 on both of these though.

Thanks a lot! I'm really happy to hear that. I'm waiting for a review from another committer and hoping everything will be fine here.

oh I just noticed the integration tests on GH are running with snappy compression, we should change it to lz4 so we get coverage on compression+v5

Oh, yeah. I'll try to change CI to run tests here.

@worryg0d
Copy link
Author

worryg0d commented Oct 31, 2024

I changed CI here to run tests over proto 5. One thing I had to replace lz4 dependency in go.mod to use the updated version of the pkg...

…CacheUsesOverriddenKeyspace integration tests when running on proto < 5 because they requires overridding keyspace feature that is available for proto 5 and higher
@joao-r-reis joao-r-reis changed the title CASSGO-1 Native Protocol 5 Support CASSGO-1 CASSGO-30 Native Protocol 5 Support Nov 6, 2024
@joao-r-reis
Copy link
Contributor

Hmm it does look weird to have gocql's go.mod list gocql/lz4 as a dependency when gocql/lz4's go.mod lists gocql as a dependency... A possible solution is to make lz4 a package instead of its own module, I brought this up in slack and here.

If lz4 stays a separate module then I'm not sure what is the best way to use lz4 in the integration tests... Maybe your approach is the best one, if I'm not mistaken the lz4 dependency will not actually be downloaded by users since it's only used in tests.

go.mod Show resolved Hide resolved
Copy link

@ribaraka ribaraka left a comment

Choose a reason for hiding this comment

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

lgtm

@worryg0d
Copy link
Author

@joao-r-reis I adjusted the CI in this PR to run integration tests over proto 5 and the lz4 compressor. Could you please approve running the workflow to see if nothing breaks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol_v5 Tickets related to supporting protocol version 5 semver-major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants