-
Notifications
You must be signed in to change notification settings - Fork 990
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
Support MySQL Compressed Protocol #787
Support MySQL Compressed Protocol #787
Conversation
cc @lance6716 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll review later, quite busy these days. Do you have spare time to review @atercattus
@@ -112,6 +112,12 @@ func ConnectWithDialer(ctx context.Context, network string, addr string, user st | |||
return nil, errors.Trace(err) | |||
} | |||
|
|||
if c.ccaps&CLIENT_COMPRESS > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should notify the user about their priority at the comment of SetCapability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest lgtm
var err error | ||
switch c.Compression { | ||
case MYSQL_COMPRESS_ZLIB: | ||
c.compressedReader, err = zlib.NewReader(c.reader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we unify the c.compressedReader
with c.reader
by interface?
} else if n != len(data) { | ||
return errors.Wrapf(ErrBadConn, "Write failed. only %v bytes written, while %v expected", n, len(data)) | ||
} | ||
c.compressedReader = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to set it to nil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On line 138:
if c.compressedReader != nil {
if err := c.ReadPacketTo(buf, c.compressedReader); err != nil {
return nil, errors.Trace(err)
}
} else {
I could move the c.compressedReader = nil
line to here (line 122) if that would make things more obvious:
if uncompressedLength > 0 {
var err error
switch c.Compression {
case MYSQL_COMPRESS_ZLIB:
c.compressedReader, err = zlib.NewReader(c.reader)
case MYSQL_COMPRESS_ZSTD:
c.compressedReader = zstd.NewReader(c.reader)
}
if err != nil {
return nil, err
}
}
The payload of a compressed packet is not always actually compressed. Payloads of less than 50 bytes are often uncompressed as the compression overhead isn't worth it. If the uncompressedLength
is 0 then the payload isn't actually compressed.
Closes #645, #647
Protocol docs: https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_compression.html
This supports both zlib (
CLIENT_COMPRESS
) and zstandard (CLIENT_ZSTD_COMPRESSION_ALGORITHM
).This can help with cross-AZ data transfer cost in the cloud and with slower network links.
This probably needs some unittests.
To test:
When inspecting the traffic with Wireshark I would recommend a recent build from the master branch of wireshark.