RUST-1306 Move Compression enum behind compression feature flags
#1055
Conversation
| @@ -872,15 +871,38 @@ tasks: | |||
| TOPOLOGY: sharded_cluster | |||
| - func: "run driver test suite" | |||
|
|
|||
| - name: test-compression | |||
| - name: test-zstd-compression | |||
There was a problem hiding this comment.
There's not a ton of value in testing all of the compressors together IMO, and because the compressors list in the client options is considered in priority-order, separating these into different variants makes it easier to get full coverage of each compressor when running the test suite.
| #[cfg(feature = "zlib-compression")] | ||
| use std::convert::TryInto; | ||
|
|
||
| pub(crate) mod compress; |
There was a problem hiding this comment.
I broke this up into modules to minimize the amount of conditional compilation going on. Also removed a lot of the utility enums -- probably just personal preference but I found them to be more noisy than helpful. The actual (de)compression logic is the same.
| #[allow(unused_mut)] | ||
| let mut compressors = vec![]; | ||
|
|
||
| if *SNAPPY_COMPRESSION_ENABLED { |
There was a problem hiding this comment.
These vars were removed from the config during the refactor (oops) so I don't think we were actually getting the compression test coverage we intended to. I'd like to move away from using environment variables in our test code to avoid situations like these where possible, so I just changed the logic to turn on compression unconditionally when a feature flag is set.
| // compression behavior is tested by running the driver test suite with a compressor configured; | ||
| // this test just makes sure our setup is correct. | ||
| #[tokio::test] | ||
| async fn test_compression_enabled() { |
There was a problem hiding this comment.
This is just adding a little bit of safeguarding to avoid accidentally not running tests with compression enabled in the future 🙂 I removed all of the other compression-related tests because they were either a) testing the functionality of our compression dependencies rather than our own code or b) just doing a round-trip.
| @@ -315,8 +320,11 @@ pub(crate) struct Handshaker { | |||
| /// given the same pool options, so it can be created at the time the Handshaker is created. | |||
| command: Command, | |||
|
|
|||
| // This field is not read without a compression feature flag turned on. | |||
| #[allow(dead_code)] | |||
| #[cfg(any( | |||
There was a problem hiding this comment.
We could avoid having to repeat this list everywhere (and update it everywhere if we add a new compressor) by making the *-compression features enable a compression feature. Up to you if you think that's worthwhile.
There was a problem hiding this comment.
Yeah, the repetition is annoying. I think it's manageable enough for now that it doesn't feel worth adding a technically-public feature flag, but I would definitely consider that if we do more compression-related work in the future.
It'd be really nice if we could define some kind of #[compression] attribute that expanded to this conditional compilation expression, but I don't think that's possible after cursory investigation of proc macro capabilities.
No description provided.