RUST-54 Support for OP_COMPRESSED #476
Conversation
|
|
| Err(Error::new( | ||
| ErrorKind::InvalidResponse { | ||
| message: format!( | ||
| "Invalid op code, expected {} or {} and got {}", |
There was a problem hiding this comment.
I think this could cause a confusing error message if the server sends a compressed message but the client doesn't have the feature flags enabled ("Invalid op code, expected <Message> or <Compressed> and got <Compressed>").
There was a problem hiding this comment.
Good point. It is confusing because if none of the feature flags are enabled then the client is not expecting OpCode::Compressed, so we shouldn't say that it is expecting OpCode::Compressed. Although the server will never send compressed messages if none of the feature flags are enabled (because the server only sends compressed messages if we tell it we can parse them).
| /// For compressors that take a level, use None to indicate the default level. | ||
| /// Requires one of zstd-compression, zlib-compression, or snappy-compression | ||
| /// feature flags. | ||
| pub enum Compressor { |
There was a problem hiding this comment.
I think a lot of the feature flag annotations could be simplified by including Noop in this enum and unconditionally including the machinery elsewhere, relying on there being only a single enum value when none are enabled.
Alternatively, the entire compression module could be put behind a
#[cfg(any(
feature = "zstd-compression",
feature = "zlib-compression",
feature = "snappy-compression"
))]
clause in src/lib.rs.
src/cmap/conn/mod.rs
Outdated
| @@ -263,8 +288,9 @@ impl Connection { | |||
| command: Command, | |||
| request_id: impl Into<Option<i32>>, | |||
| ) -> Result<RawCommandResponse> { | |||
| let to_compress: bool = command.should_compress(); | |||
There was a problem hiding this comment.
Do we need a type annotation here?
| ], | ||
| "zlibCompressionLevel": 9 | ||
| } | ||
| "options": {} |
There was a problem hiding this comment.
Why were these removed -- was there an update to the spec tests that we pulled in here?
There was a problem hiding this comment.
The reason is that we only store compressors that are valid and enabled (via its corresponding feature flag). When these tests are run none of the feature flags are set, so none of these compressors get stored. As a result, when ClientOptions is deserialized, all these fields are empty. We probably want to also run these tests with the feature flags set.
There was a problem hiding this comment.
I'll just disable these tests if not all of the feature flags are set. I didn't realize that we don't manually edit these.
| @@ -0,0 +1,127 @@ | |||
| // Tests OP_COMPRESSED. To actually test compression you need to look at | |||
There was a problem hiding this comment.
Are the tests in this module actually being run in CI? I believe we'd need to update our evergreen config to enable the compression feature flags, from looking at the evergreen logs it doesn't look like these ones show up in the test output.
There was a problem hiding this comment.
No they aren't being run right now. They only run if the feature flags are set. I was going to update the evergreen configs in a separate PR.
awitten1
force-pushed
the
RUST-54_OP-COMPRESSED
branch
from
b19f3d3 to
de04ddb
Compare
Provides support for reading and writing OP_COMPRESSED as specified here. Includes support for snappy, zstd, and zlib.
The following is a snippet of the server logs when running
cargo test compression::test::ping_server_with_zlib_compression, which connects to the default server (seeget_default_serverin src/test/mod.rs) and sends a ping (see src/compression/test.rs). I have highlighted the most relevant parts. (The exact output will depend on the server version).The following is a snippet of the server logs where the server has been given the flag
--networkMessageCompressors=snappy,zstd(prohibiting usage of zlib) and runningcargo test compression::test::ping_server_with_all_compressor, which lists all compressors as usable on the client side. The client compresses with snappy, as it should, because snappy is preferred over zstd, and zlib is not usable.