Conversation
Previously, the driver encoded each individual frame in a single segment despite segments ability to hold multiple frames. This patch introduces a mechanism that allows driver collect multiple frames before encoding them as a batch. To achieve this, segmentWriter was introduced. Patch by Bohdan Siryk; reviewed by TBD for CASSGO-100
Prevent write hang on segmentWriter.writeContext call when writer is closed. Prevent stale timer tick when current segment is about to be flushed because new request doesn't fit current segment
There was a problem hiding this comment.
Pull request overview
This PR refactors and formalizes Native Protocol v5 “segment” framing by extracting segment encode/decode logic into a dedicated codec and wiring a new segment-based reader/writer into connection startup, replacing the prior inline segment implementation.
Changes:
- Added
segmentCodec(compressed + uncompressed) with CRC validation and segment payload handling. - Introduced
segmentWriter/segmentReaderand switched connections to segment-based I/O after proto v5 startup completes. - Added extensive unit tests and benchmarks for segment encoding/decoding and segment-based I/O; removed legacy segment helpers/tests from
frame.go/frame_test.go.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| segment_codec.go | New segment encode/decode implementation (CRC24 header, CRC32 payload, compressed/uncompressed formats). |
| segment_codec_test.go | New unit tests + benchmarks for segmentCodec behavior (CRC failures, roundtrips, worthiness). |
| protocol_negotiation_test.go | Removes debug logger wiring from test setup. |
| frame.go | Removes legacy segment framing helpers/constants from the frame layer. |
| frame_test.go | Removes legacy segment tests and now-obsolete helpers/imports. |
| control.go | Routes internal control-path requests via execInternal. |
| conn.go | Adds segmentReader/segmentWriter and switches to them post-startup for proto v5; refactors startup coordinator accordingly. |
| conn_test.go | Updates tests for new recv signature and adds tests for segment reader/writer behavior. |
| cassandra_test.go | Removes debug logger wiring, updates exec callsite, and fixes table name used in compression-worthiness test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| err := sw.encodeAndWrite(sw.frames, true) | ||
| if err != nil { | ||
| sw.failPending(fmt.Errorf("error occured while encoding and writing of the current segment: %w", err)) |
Comment on lines
+2293
to
+2300
| frameHeader, err := readHeader(bytes.NewBuffer(payload), sr.frameHeaderBuf[:]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Allocate a buffer to read the rest of the segment into | ||
| buf := bytes.NewBuffer(make([]byte, 0, frameHeader.length+frameHeadSize)) | ||
| buf.Write(payload) |
Comment on lines
+2161
to
+2164
| if flushErr == nil { | ||
| sw.w.SetWriteDeadline(time.Now().Add(sw.writeTimeout)) | ||
| _, flushErr = segments.WriteTo(sw.w) | ||
| } |
Comment on lines
+2188
to
+2192
| sw.w.SetWriteDeadline(time.Now().Add(sw.writeTimeout)) | ||
| _, err = sw.w.Write(segmentBuf) | ||
| if err != nil { | ||
| return err | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.