-
Notifications
You must be signed in to change notification settings - Fork 2k
chore(flags): Add compression and serialization configuration to Redis clients #40764
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
base: master
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds zstd compression support to the Redis client library to match Django's compression behavior. The implementation allows for transparent compression/decompression of Redis values based on configurable thresholds.
- Adds
CompressionConfigstruct with Django-compatible defaults (512-byte threshold, level 0) - Implements compression/decompression logic that gracefully handles both compressed and uncompressed data
- Updates
RedisClientto store and use compression and format configurations
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rust/common/redis/src/lib.rs | Adds compression infrastructure, configuration types, helper methods, and comprehensive test coverage |
| rust/common/redis/Cargo.toml | Adds zstd dependency version 0.13 |
| rust/Cargo.lock | Updates lock file with zstd dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
eae6603 to
10e2bb9
Compare
10e2bb9 to
535386f
Compare
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.
2 files reviewed, 2 comments
I don't want to break other consumers of this library.
Addresses feedback from PR review: - Always call `try_decompress()` on reads, regardless of compression config - This allows clients to read data written with different compression settings - Prevents "config mismatch" issues where data becomes unreadable Also improves error detection: - Add logging via tracing to detect corrupted compressed data - Check for zstd magic bytes to distinguish corruption from uncompressed data - Log warning only when decompression fails on data with compression headers
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.
Nice, I just noticed that the description of the PR mentions that CompressionConfig is enabled by default and you've changed that, so you might want to change there also.
Problem
The Rust
RedisClientneeds to be compatible with Django's Redis caching behavior for seamless interoperability between Python and Rust services. Currently:Zstdcompression for large values (>512 bytes) to reduce Redis memory usage and network overheadWithout these features, Rust services cannot properly read/write data that Django services cache, and vice versa.
Changes
Added configurable compression and serialization format support to
RedisClientwith Django-compatible defaults:Compression Support (Zstd)
CompressionConfig- Configures compression behavior with Django-compatible defaults:enabled: true- Compression on by defaultthreshold: 512- Matches Django'sZstdCompressor.min_lengthlevel: 0- Matches Django's default preset (equivalent to level 3)try_decompress()handles both compressed and uncompressed data, allowing gradual rolloutSerialization Format Support
RedisValueFormatenum - Three formats for different use cases:Pickle(default) - Django-compatible serializationUtf8- Simple string encodingRawBytes- Direct binary data (via dedicated methods)*_with_format()methods available when neededAPI Design
Simplified constructor API:
How did you test this code?
Tests cover:
Backwards Compatible
Existing code is not affected as defaults remain the same. Added a new
ctorfor configuring the client.