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

worryg0d · 2024-09-16T08:01:11Z

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:

New frames format CASSANDRA-15299
result_metadata_id field for EXECUTE and PREPARED messages CASSANDRA-10786
now_in_seconds field for BATCH and QUERY messages CASSANDRA-14664
keyspace field for BATCH and QUERY messages

worryg0d · 2024-10-01T15:01:29Z

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

joao-r-reis · 2024-10-08T15:17:34Z

What about #1783

lukasz-antoniak

Copied unsolved comments fromCopied unresolved comments from previous PRs.

frame_test.go

lukasz-antoniak · 2024-10-08T15:43:12Z

What about #1783

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

worryg0d · 2024-10-09T04:17:51Z

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 · 2024-10-09T09:29:35Z

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

worryg0d · 2024-10-10T12:36:12Z

I merged changes from #1783 into this one.

joao-r-reis

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

batch_test.go

cassandra_test.go

compressor.go

conn.go

session.go

frame.go

conn.go

worryg0d · 2024-10-15T18:01:13Z

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 · 2024-10-16T12:47:09Z

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.

ribaraka · 2024-10-17T07:20:41Z

https://issues.apache.org/jira/browse/CASSANDRA-20004

joao-r-reis · 2024-10-17T12:06:16Z

I'm reviewing the protocol/envelope changes for the new protocol v5 frame format and one thing I'm noticing is that the name frame can either reference the "old frame" (called envelope in v5) or the "new frame" and you can't really tell easily which one it is when reading a particular function that references that name (one has to read the "surrounding" code to understand where it is being called and if it refers to the protocol v5 format or not).

In the java driver this was avoided by calling the new v5 frame a "segment" while "frame" essentially always refers to the envelope (so "frame" remains the same as a concept). It's not ideal to use a word that is not even referenced in the protocol spec but I think having this clear distinction between the two would be very helpful to make the code easier to read (also prevents bugs by someone conflating the two while modifying the code).

worryg0d · 2024-10-17T12:18:36Z

I'm reviewing the protocol/envelope changes for the new protocol v5 frame format and one thing I'm noticing is that the name frame can either reference the "old frame" (called envelope in v5) or the "new frame" and you can't really tell easily which one it is when reading a particular function that references that name (one has to read the "surrounding" code to understand where it is being called and if it refers to the protocol v5 format or not).

In the java driver this was avoided by calling the new v5 frame a "segment" while "frame" essentially always refers to the envelope (so "frame" remains the same as a concept). It's not ideal to use a word that is not even referenced in the protocol spec but I think having this clear distinction between the two would be very helpful to make the code easier to read (also prevents bugs by someone conflating the two while modifying the code).

I agree because I had the same problems distinguishing frames and envelopes. I wanted to call it another way, but according to proto spec, it should be called as frames so I decided to leave this part until it got reviewed. Then we could call it Segment instead of frame. Also, it makes it easy for other contributors (e.a. from java-driver) to distinguish them.

joao-r-reis

Looking good, added a few comments. Still working on the v5 frame format review (I'm reading up on the format spec and also reading some java driver code for comparison)

conn.go

joao-r-reis · 2024-10-17T13:00:56Z

conn.go

+	// 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{}


This will cause the minimum Go version to be 1.19 (instead of the current 1.17 according to my experiments). I raised this on the ASF slack channel to see if people think it's ok otherwise we might have to use atomic.Value instead...

Oops, I missed it. It's kinda a shame that we can't use more distinguishing atomic types instead of generic Value... However, the code base already has some similar things, so changing it is not a very big problem.

Yeah I'd rather use atomic.Bool and 1.19 is 3 versions behind what we currently officially support anyway so personally I don't think it is a big deal to bump the version on go.mod to 1.19 but let's see if others chime in

Then I'll leave it by the time when we receive some feedback on it

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

I think forcing 1.19 is fine.

frame.go

joao-r-reis

Most of my comments are NITs related to code readability. Feel free to close any of these specific ones that you don't agree with. The implementation itself looks very solid 👍

conn.go

frame.go

worryg0d · 2024-11-12T16:03:39Z

@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?

conn.go

worryg0d · 2025-02-13T15:26:59Z

Hello. This PR has already been floating for quite a while. Is there anything I can do to push it forward?

joao-r-reis · 2025-02-17T13:21:13Z

Hello. This PR has already been floating for quite a while. Is there anything I can do to push it forward?

I'd like to get a +1 from another committer before merging this one because of its size. I think @jameshartig will take a look soon

conn.go

jameshartig · 2025-02-18T15:30:32Z

go.mod

+	gopkg.in/yaml.v3 v3.0.1 // indirect
+)
+
+replace github.com/gocql/gocql/lz4 => ./lz4


I don't think this is needed. Go should know what the module is based on the module line.

When I was trying to run tests over lz4 pkg with changes I faced compilation errors. It looks like go uses vendored cassandra-gocql-driver/lz4 instead of local, so I decided to add this. There is PR #1842 that moves lz4 to gocql module. If it is merged first, It will enable me to remove this replace.

Yeah I think we should merge #1842 first, can you take a quick look @jameshartig ? It is very small

That should be good to merge

.github/workflows/main.yml

jameshartig · 2025-02-18T15:35:04Z

@worryg0d apologies for the delay. I only had a few comments. I'm mostly concerned about not testing the old protocol and compressor for anyone not using proto 5.

worryg0d · 2025-02-18T16:38:24Z

@jameshartig Thanks a lot for the feedback! I'll try to update the CI matrix to run tests on proto 4 + snappy and proto 5 + lz4

worryg0d · 2025-02-20T13:48:12Z

Hello @jameshartig @joao-r-reis. I added to the CI matrix proto version and snappy compressor to run tests over both versions and compressors.

I also squashed all commits and changed a commit message.

One thing I left is replacing in go.mod until #1842 is merged.

Could you please review the CI changes and approve the workflow run? Thanks!

ribaraka · 2025-02-21T08:08:34Z

cassandra_test.go

+	// Forcing to run this test without any compression.
+	// If compressor is presented, then CI will fail when snappy compression is enabled, since
+	// protocol v5 doesn't support it.
+	cluster.Compressor = nil


You've exluded the snappy from proto v5 in the CI, then everyting should work fine. This block of code is likely unnecessary, isn't it?

This test verifies the protocol discovery mechanism by explicitly setting cluster.ProtoVersion = 0, so it means that this test doesn't depend on the CI matrix.

The idea behind protocol discovery is to send a query to C* with the newest protocol version that the driver supports, which in this case is proto 5. For instance, the latest proto version supported by Cassandra 3 is proto 4, but the driver supports 5. The driver will send a query with proto 5 version, and C* 3 will return an error with the lowest and greatest proto versions. Then the driver picks the greatest one, which is v4 for C* 3.

The issue comes when CI runs this test with matrix.proto_version = 4 and matrix.compressor = snappy. Proto version is ignored, but not the compressor. So, the driver will send queries with max proto version of 5 and snappy compressor, which causes Cassandra to return an error about unsupported compressor type for protocol v5.

ribaraka · 2025-02-21T10:12:44Z

cassandra_test.go

+func TestPrepareExecuteMetadataChangedFlag(t *testing.T) {
+	// This test ensures that the whole Metadata_changed flow
+	// is handled properly.
+	//
+	// To trigger C* to return Metadata_changed we should do:
+	// 1. Create a table
+	// 2. Prepare stmt which uses the created table
+	// 3. Change the table schema in order to affect prepared stmt (e.g. add a column)
+	// 4. Execute prepared stmt. As a result C* should return RESULT/ROWS response with
+	//    Metadata_changed flag, new metadata id and updated metadata resultset.
+	//
+	// The driver should handle this by updating its prepared statement inside the cache
+	// when it receives RESULT/ROWS with Metadata_changed flag


Could you move descriptions above the function declaration?

ribaraka · 2025-02-21T10:19:21Z

The updated CI looks goot to me.

jameshartig

LGTM and kicked off CI

worryg0d · 2025-02-24T08:07:33Z

Thanks a lot!

I also noticed that CI doesn't run tests without any compression. Should I add it to the matrix? With proto v5 it makes sense to add them because we have different segment format for compressed and uncompressed segments

Native Protocol 5 was introduced with the release of C* 4.0. This PR provides full support for a newer version including new format frames (segments), and new fields for QUERY, BATCH, and EXECUTE messages. Also, this PR brings changes to the Compressor interface to follow an append-like design. One more thing, it bumps Go version to the newer 1.19. Patch by Bohdan Siryk; Reviewed by João Reis, James Hartig for CASSGO-1 CASSGO-30

worryg0d · 2025-02-27T15:27:00Z

Fixed conflicts caused by #1842 and deleted replace in go.mod

worryg0d mentioned this pull request Sep 16, 2024

Support for result_metadata_id in PREPARED and EXECUTE messages #1782

Closed

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 mentioned this pull request Sep 19, 2024

Proto v5 support was added #1773

Closed

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 added the protocol_v5 Tickets related to supporting protocol version 5 label Oct 1, 2024

This was referenced Oct 1, 2024

Support for now_in_seconds option field for QUERY, BATCH and EXECUTE messages #1781

Closed

Support of keyspace field for BATCH message #1778

Closed

lukasz-antoniak reviewed Oct 8, 2024

View reviewed changes

frame_test.go Show resolved Hide resolved

worryg0d force-pushed the native-proto-5-support-all-merged branch 2 times, most recently from 3f50a90 to fbdaa2b Compare October 10, 2024 12:14

tengu-alt mentioned this pull request Oct 15, 2024

CASSGO-11 Support vector type #1828

Open

joao-r-reis reviewed Oct 15, 2024

View reviewed changes

joao-r-reis reviewed Oct 17, 2024

View reviewed changes

joao-r-reis reviewed Oct 18, 2024

View reviewed changes

This was referenced Nov 20, 2024

CASSGO-32 move lz4 compressor to lz4 package within the gocql module #1842

Merged

WIP-Cassgo 33 snappy has been moved to a separate package #1844

Open

testisnullus approved these changes Jan 24, 2025

View reviewed changes

joao-r-reis mentioned this pull request Jan 29, 2025

CASSGO-40: skip metadata only if prepared result included metadata #1852

Merged

jameshartig reviewed Jan 29, 2025

View reviewed changes

conn.go Show resolved Hide resolved

jameshartig reviewed Jan 29, 2025

View reviewed changes

conn.go Outdated Show resolved Hide resolved

jameshartig reviewed Feb 18, 2025

View reviewed changes

conn.go Show resolved Hide resolved

jameshartig reviewed Feb 18, 2025

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

jameshartig reviewed Feb 18, 2025

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

OleksiienkoMykyta mentioned this pull request Feb 20, 2025

Include hash of result set metadata in prepared statement id #1222

Closed

worryg0d force-pushed the native-proto-5-support-all-merged branch 2 times, most recently from 80e9056 to f6b73d9 Compare February 20, 2025 13:44

OleksiienkoMykyta mentioned this pull request Feb 21, 2025

can not change column type dynamically #1186

Closed

ribaraka reviewed Feb 21, 2025

View reviewed changes

worryg0d force-pushed the native-proto-5-support-all-merged branch from f6b73d9 to 549f36c Compare February 21, 2025 12:42

jameshartig approved these changes Feb 22, 2025

View reviewed changes

This was referenced Feb 26, 2025

Cassandra 4.0 support ? #1521

Open

Support new frame format in protocol V5 #1522

Open

worryg0d force-pushed the native-proto-5-support-all-merged branch from 549f36c to a7aa3c5 Compare February 27, 2025 15:22

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

Are you sure you want to change the base?

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

Conversation

worryg0d commented Sep 16, 2024 • edited Loading

Overview

worryg0d commented Oct 1, 2024 • edited Loading

joao-r-reis commented Oct 8, 2024

lukasz-antoniak left a comment

Choose a reason for hiding this comment

lukasz-antoniak commented Oct 8, 2024

worryg0d commented Oct 9, 2024

testisnullus commented Oct 9, 2024

worryg0d commented Oct 10, 2024

joao-r-reis left a comment

Choose a reason for hiding this comment

worryg0d commented Oct 15, 2024

worryg0d commented Oct 16, 2024

ribaraka commented Oct 17, 2024

joao-r-reis commented Oct 17, 2024

worryg0d commented Oct 17, 2024

joao-r-reis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joao-r-reis left a comment

Choose a reason for hiding this comment

worryg0d commented Nov 12, 2024

worryg0d commented Feb 13, 2025

joao-r-reis commented Feb 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameshartig commented Feb 18, 2025

worryg0d commented Feb 18, 2025

worryg0d commented Feb 20, 2025

ribaraka Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ribaraka commented Feb 21, 2025

jameshartig left a comment

Choose a reason for hiding this comment

worryg0d commented Feb 24, 2025

worryg0d commented Feb 27, 2025

worryg0d commented Sep 16, 2024 •

edited

Loading

worryg0d commented Oct 1, 2024 •

edited

Loading

ribaraka Feb 21, 2025 •

edited

Loading