joao-r-reis commented on code in PR #1822:
URL:
https://github.com/apache/cassandra-gocql-driver/pull/1822#discussion_r1809044319
##########
go.mod:
##########
@@ -23,7 +23,7 @@ require (
github.com/golang/snappy v0.0.3
github.com/hailocab/go-hostpool v0.0.0-20160125115350-e80d13ce29ed
github.com/kr/pretty v0.1.0 // indirect
- github.com/stretchr/testify v1.3.0 // indirect
+ github.com/stretchr/testify v1.9.0
Review Comment:
do we use this library directly? it used to be just an indirect dependency
##########
conn.go:
##########
@@ -694,7 +718,7 @@ func (c *Conn) recv(ctx context.Context) error {
} else if head.stream == -1 {
// TODO: handle cassandra event frames, we shouldnt get any
currently
framer := newFramer(c.compressor, c.version)
- if err := framer.readFrame(c, &head); err != nil {
+ if err := framer.readFrame(r, &head); err != nil {
return err
}
go c.session.handleEvent(framer)
Review Comment:
> Something like
[this](https://github.com/worryg0d/gocql/commit/44cc2fc9f2344928f261d43c60071db1170d706c)?
It makes sense since it forces us to use c.r instead of Conn itself. However,
there is net.Conn still presented in Conn, which may be accidentally used
instead.
Yeah, we can go even further and completely move both `net.Conn` and
`bufio.Reader` fields to a separate type that implements the `net.Conn`
interface. the `timeout` field could also be removed from `gocql.Conn`. This
way we ensure nobody uses the underlying `net.Conn` object directly by mistake.
```
type ConnWrapper interface {
net.Conn
}
type connWrapper struct {
conn net.Conn
r *bufio.Reader
timeout time.Duration
}
func (c *connWrapper) Read(p []byte) (n int, err error) {
const maxAttempts = 5
for i := 0; i < maxAttempts; i++ {
var nn int
if c.timeout > 0 {
c.SetReadDeadline(time.Now().Add(c.timeout))
}
nn, err = io.ReadFull(c.r, p[n:])
n += nn
if err == nil {
break
}
if verr, ok := err.(net.Error); !ok || !verr.Temporary() {
break
}
}
return
}
// implement remaining net.Conn`functions by just delegating to c.conn
```
##########
frame.go:
##########
@@ -2070,3 +2133,247 @@ func (f *framer) writeBytesMap(m map[string][]byte) {
f.writeBytes(v)
}
}
+
+func (f *framer) prepareModernLayout() error {
+ // Ensure protocol version is V5 or higher
+ if f.proto < protoVersion5 {
+ panic("Modern layout is not supported with version V4 or less")
+ }
+
+ selfContained := true
+
+ var (
+ adjustedBuf []byte
+ tempBuf []byte
+ err error
+ )
+
+ // Process the buffer in chunks if it exceeds the max payload size
+ for len(f.buf) > maxPayloadSize {
+ if f.compres != nil {
+ tempBuf, err =
newCompressedFrame(f.buf[:maxPayloadSize], false, f.compres)
+ } else {
+ tempBuf, err =
newUncompressedFrame(f.buf[:maxPayloadSize], false)
+ }
+ if err != nil {
+ return err
+ }
+
+ adjustedBuf = append(adjustedBuf, tempBuf...)
Review Comment:
Hmm that implementation has less allocations but it has an issue. If the
request is large and "generates" multiple segments then it will wait until the
write coalescer writes 1 segment before writing the next one.
I think we can keep the existing framer based implementation for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
