-
Notifications
You must be signed in to change notification settings - Fork 92
fix(compression-coordinator): Restrict Zstandard detection to RFC-compliant .zst suffixes (fixes #2414); match suffixes case-insensitively.
#2419
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
Changes from 10 commits
6e81528
a1b6015
3c66a96
876e7b6
cfc2460
8ee265f
c5b52dc
5d64747
268d988
20522ef
2fa5efd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Bill-hbrhbr marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -240,19 +240,30 @@ impl Iterator for RoundRobinIterator { | |
| /// The estimated uncompressed size. | ||
| fn estimate_uncompressed_size(key: &str, size: u64) -> u64 { | ||
| const GZIP_COMPRESSION_RATIO_ESTIMATE: u64 = 13; | ||
| const GZIP_SUFFIXES: &[&str] = &[".gz", ".gzip", ".tgz", ".tar.gz"]; | ||
| const GZIP_SUFFIXES: &[&str] = &[".gz", ".gzip", ".tgz"]; | ||
| const ZSTD_COMPRESSION_RATIO_ESTIMATE: u64 = 8; | ||
| const ZSTD_SUFFIXES: &[&str] = &[".zstd", ".zstandard", ".tar.zstd", ".tar.zstandard"]; | ||
|
|
||
| if GZIP_SUFFIXES.iter().any(|suffix| key.ends_with(suffix)) { | ||
| if GZIP_SUFFIXES | ||
| .iter() | ||
| .any(|suffix| ends_with_ignore_ascii_case(key, suffix)) | ||
| { | ||
| size * GZIP_COMPRESSION_RATIO_ESTIMATE | ||
| } else if ZSTD_SUFFIXES.iter().any(|suffix| key.ends_with(suffix)) { | ||
| } else if ends_with_ignore_ascii_case(key, ".zst") { | ||
| size * ZSTD_COMPRESSION_RATIO_ESTIMATE | ||
| } else { | ||
| size | ||
| } | ||
| } | ||
|
|
||
| /// # Returns | ||
| /// | ||
| /// Whether `key` ends with `suffix` using ASCII case-insensitive match. | ||
| fn ends_with_ignore_ascii_case(key: &str, suffix: &str) -> bool { | ||
| let suffix_start = key.len().saturating_sub(suffix.len()); | ||
| let key_suffix = key.get(suffix_start..); | ||
| key_suffix.is_some_and(|key_suffix| key_suffix.eq_ignore_ascii_case(suffix)) | ||
| } | ||
|
|
||
| /// Gets the filename portion of an S3 object's key. | ||
| /// | ||
| /// # Returns | ||
|
|
@@ -393,6 +404,38 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_estimate_uncompressed_size_for_gzip_suffix() { | ||
| const FILE_SIZE: u64 = 100; | ||
|
|
||
| for path in [ | ||
| "logs/app.log.gz", | ||
| "logs/app.log.GZ", | ||
| "logs/app.log.gzip", | ||
| "logs/app.log.GZIP", | ||
| "logs/app.log.tgz", | ||
| "logs/app.log.TGZ", | ||
| "logs/app.log.tar.gz", | ||
| "logs/app.log.TAR.GZ", | ||
| ] { | ||
| assert_eq!(FILE_SIZE * 13, estimate_uncompressed_size(path, FILE_SIZE)); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_estimate_uncompressed_size_for_zstandard_suffix() { | ||
| const FILE_SIZE: u64 = 100; | ||
|
|
||
| for path in [ | ||
| "logs/app.log.zst", | ||
| "logs/app.log.clp.zst", | ||
| "logs/app.log.tar.zst", | ||
| "logs/app.log.ZST", | ||
| ] { | ||
| assert_eq!(FILE_SIZE * 8, estimate_uncompressed_size(path, FILE_SIZE)); | ||
| } | ||
| } | ||
|
Comment on lines
+425
to
+437
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Cover the full Zstandard suffix contract in both test suites. Both suites verify supported
📍 Affects 2 files
🤖 Prompt for AI Agents
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
i'm not sure if that's a must. seems like adding such tests would also only add maintenance debt rather than actual value. what do you think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
They protect the main behavioral boundary of Since the four cases directly correspond to the removed aliases (including their 🐇💭 |
||
|
|
||
| #[test] | ||
| fn test_partition_on_flush() { | ||
| const TARGET_ARCHIVE_SIZE: u64 = 1000; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.