Proto v5 support was added #1773

testisnullus · 2024-06-24T11:57:13Z

The pull request involves adding support for Protocol Version 5 (Proto v5) and addresses several key issues related to compatibility and the implementation of new features introduced in Cassandra's native protocol.

Key Issues and Changes in the PR:

New Frame Format Support. The PR implements changes to support the new frame format introduced in Proto v5. This format includes modifications that are not backward-compatible with previous versions, requiring updates to the driver's handling of message frames to interpret and process the protocol's data streams correctly. Also, this PR added a logic to process the CRC calculations that ensures reliable communication between the driver and Cassandra nodes using Proto v5.
Backward Compatibility. Ensuring backward compatibility with older protocol versions while implementing Proto v5 was a significant focus. These changes ensure that the driver works correctly with the latest Cassandra versions, particularly those utilizing the newer features and optimizations of Proto v5.

…E message

lukasz-antoniak · 2024-08-26T10:09:37Z

conn.go

@@ -660,6 +673,16 @@ func (c *Conn) heartBeat(ctx context.Context) {
 }

 func (c *Conn) recv(ctx context.Context) error {
+	// If native proto v5+ is used and conn is set up, then we should
+	// unwrap payload body from v5 compressed/uncompressed frame
+	if c.version > protoVersion4 && c.connReady {


Maybe revert the condition. If connection has not been initialised, we should not relay on protocol version.

If I understand correctly you meant to change the order. Could you please add some details here?

if c.startupCompleted && c.version > protoVersion4 { ... }

lukasz-antoniak · 2024-08-26T11:55:45Z

conn.go

@@ -215,6 +216,14 @@ type Conn struct {
 	host            *HostInfo
 	isSchemaV2      bool

+	// Only for proto v5+.
+	// Indicates if Conn is ready to use Native Protocol V5.


This flag indicates that STARTUP message has been completed, not that we can use V5. Please fix the comment, because it can be misleading.

I agree. I changed the comment and renamed the flag to startupCompleted.

// Only for proto v5+. // Indicates if STARTUP has been completed. // github.com/apache/cassandra/blob/trunk/doc/native_protocol_v5.spec // 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 bool

lukasz-antoniak · 2024-08-26T12:14:57Z

conn.go

-			params:        params,
-			customPayload: qry.customPayload,
+			preparedID:         info.id,
+			preparedMetadataID: info.metadataID,


Rename the field to result_metadata_id. preparedMetadataID can be misleading.

lukasz-antoniak · 2024-08-26T12:22:57Z

conn.go

-			customPayload: qry.customPayload,
+			preparedID:         info.id,
+			preparedMetadataID: info.metadataID,
+			params:             params,


resultRowsFrame does not handle new_metadata_id and does not propagate it to next iterator calls.

<new_metadata_id> is [short bytes] representing the new, changed resultset metadata. The new metadata ID must also be used in subsequent executions of the corresponding prepared statement, if any.

This is needed to solve CASSANDRA-10786.

lukasz-antoniak · 2024-08-26T12:25:06Z

conn.go

-	request  preparedMetadata
-	response resultMetadata
+	id         []byte
+	metadataID []byte


Please change the name to result_metadata_id as in the protocol not to create confusion.

lukasz-antoniak · 2024-08-27T09:58:15Z

frame.go

+		return nil, fmt.Errorf("payload length (%d) exceeds maximum size of 128 KiB", payloadLen)
+	}
+
+	header := make([]byte, headerSize)


Can we here not create header array and then copy it over to frame, but instead just write to frame?

Good catch! I moved the frame buffer initializing to allow the header to be written directly into the frame.

This has been addressed in #1822.

lukasz-antoniak · 2024-08-27T10:16:23Z

frame.go

+		// 2.2
+		//  An uncompressed length of 0 signals that the compressed payload
+		//  should be used as-is and not decompressed.
+		compressedPayload = uncompressedPayload


TestLargeSizeQuery will generate random text, which LZ4 compressor most likely will not encode to smaller representation. Create a test to make sure that LZ4 compression is applied and we do not fall under this condition.

I agree, I added a test that covers this case as well.

lukasz-antoniak · 2024-08-27T10:58:01Z

frame.go

+	return buf.Bytes(), nil
+}
+
+func readCompressedFrame(r io.Reader, compressor Compressor) ([]byte, bool, error) {


We are missing tests to simulate various frame errors here, e.g. invalid length encoded in the frame, invalid CRC. Tests could use our frame building logic for a simple EXECUTE or QUERY, change payload and then make sure decoding fails with certain error.

I agree and added tests for readCompressedFrame and readUncompressedFrame.

lukasz-antoniak · 2024-08-27T11:21:32Z

conn.go

@@ -1394,9 +1483,10 @@ func (c *Conn) executeQuery(ctx context.Context, qry *Query) *Iter {
 		params.skipMeta = !(c.session.cfg.DisableSkipMetadata || qry.disableSkipMetadata)

 		frame = &writeExecuteFrame{


For BATCH message we are missing keyspace field.

0x0080: With keyspace. If set, <keyspace> must be present. <keyspace> is a [string] indicating the keyspace that the query should be executed in. It supercedes the keyspace that the connection is bound to, if any.

lukasz-antoniak · 2024-08-27T11:26:36Z

conn.go

@@ -1394,9 +1483,10 @@ func (c *Conn) executeQuery(ctx context.Context, qry *Query) *Iter {
 		params.skipMeta = !(c.session.cfg.DisableSkipMetadata || qry.disableSkipMetadata)

 		frame = &writeExecuteFrame{


Field <now_in_seconds> has not been added as per below quote and flags bitmap not updated.

Added now_in_seconds field in QUERY, EXECUTE, and BATCH messages (Sections 4.1.4, 4.1.6, and 4.1.7).

worryg0d · 2024-09-19T12:52:00Z

Thanks a lot for the feedback!

Most of the issues mentioned in this PR have been covered in my other PRs related to proto 5 support.

What do you think about continuing the discussion in PR #1822 which contains changes from all others? I tried to segregate different features on different PRs for easy review. However, it doesn't seem to be as easy as I thought.

@lukasz-antoniak What are your thoughts on this?

joao-r-reis · 2024-10-01T13:30:02Z

Can this be closed in favor of #1822 ?

worryg0d · 2024-10-01T14:55:37Z

Can this be closed in favor of #1822 ?

Replied in #1778.

worryg0d force-pushed the proto-v5-support-prettified branch from 7dba62a to 31b4ac8 Compare July 3, 2024 12:55

worryg0d force-pushed the proto-v5-support-prettified branch from 31b4ac8 to bb416bb Compare July 23, 2024 10:33

worryg0d added 3 commits July 23, 2024 13:36

Support for result_metadata_id field for PREPARED response and EXECUT…

f8007d0

…E message

Support for Native Protocol 5 new frames format

bc6a11b

CI changes to use lz4 compressor and proto 5 for tests

148bca6

worryg0d force-pushed the proto-v5-support-prettified branch from bb416bb to 148bca6 Compare July 23, 2024 10:38

absurdfarce added the protocol_v5 Tickets related to supporting protocol version 5 label Aug 21, 2024

lukasz-antoniak suggested changes Aug 27, 2024

View reviewed changes

worryg0d mentioned this pull request Oct 1, 2024

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

Open

joao-r-reis closed this Oct 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proto v5 support was added #1773

Proto v5 support was added #1773

testisnullus commented Jun 24, 2024 •

edited

Loading

lukasz-antoniak Aug 26, 2024

worryg0d Sep 19, 2024

lukasz-antoniak Aug 26, 2024

worryg0d Sep 19, 2024

lukasz-antoniak Aug 26, 2024

lukasz-antoniak Aug 26, 2024

lukasz-antoniak Aug 26, 2024

lukasz-antoniak Aug 27, 2024

worryg0d Sep 19, 2024

lukasz-antoniak Oct 8, 2024

lukasz-antoniak Aug 27, 2024

worryg0d Sep 19, 2024

lukasz-antoniak Aug 27, 2024

worryg0d Sep 19, 2024

lukasz-antoniak Aug 27, 2024

lukasz-antoniak Aug 27, 2024

worryg0d commented Sep 19, 2024

joao-r-reis commented Oct 1, 2024

worryg0d commented Oct 1, 2024

		@@ -1394,9 +1483,10 @@ func (c Conn) executeQuery(ctx context.Context, qry Query) *Iter {
		params.skipMeta = !(c.session.cfg.DisableSkipMetadata \|\| qry.disableSkipMetadata)

		frame = &writeExecuteFrame{

Proto v5 support was added #1773

Proto v5 support was added #1773

Conversation

testisnullus commented Jun 24, 2024 • 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

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

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

worryg0d commented Sep 19, 2024

joao-r-reis commented Oct 1, 2024

worryg0d commented Oct 1, 2024

testisnullus commented Jun 24, 2024 •

edited

Loading