introduce zstd for compression - #4185
Conversation
✅ Deploy Preview for zio-http ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Adds server-side zstd response compression support (Accept-Encoding negotiation via Netty HttpContentCompressor), aligning with the existing ability to accept Content-Encoding: zstd when users provide the JNI dependency.
Changes:
- Introduces
CompressionOptions.ZstdplusZstdConfigand aCompressionOptions.zstd(...)constructor. - Extends
CompressionOptions.configparsing to recognize"type" = "zstd"and read zstd-specific parameters. - Adds Netty conversion support for zstd via
StandardCompressionOptions.zstd(...).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
zio-http/shared/src/main/scala/zio/http/Server.scala |
Adds zstd compression option/config and updates config parsing to support selecting zstd. |
zio-http/jvm/src/main/scala/zio/http/netty/model/Conversions.scala |
Maps the new zstd compression option into Netty StandardCompressionOptions.zstd(...). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -414,12 +434,15 @@ object Server extends ServerPlatformSpecific { | |||
| zio.Config.int("quantity").withDefault(BrotliConfig.DefaultQuality) ++ | |||
| zio.Config.int("lgwin").withDefault(BrotliConfig.DefaultLgwin) ++ | |||
| zio.Config.string("mode").map(Mode.fromString).withDefault(BrotliConfig.DefaultMode) ++ | |||
| zio.Config.string("type") | |||
| ).map { case (level, bits, mem, quantity, lgwin, mode, typ) => | |||
| zio.Config.string("type") ++ | |||
| zio.Config.int("block").withDefault(ZstdConfig.DefaultBlockSize) ++ | |||
| zio.Config.int("maxencode").withDefault(ZstdConfig.DefaultMaxEncodeSize) | |||
| ).map { case (level, bits, mem, quantity, lgwin, mode, typ, block, maxencode) => | |||
| typ.toLowerCase match { | |||
| case "gzip" => gzip(level, bits, mem) | |||
| case "deflate" => deflate(level, bits, mem) | |||
| case "brotli" => brotli(quantity, lgwin, mode) | |||
| case "zstd" => zstd(level, block, maxencode) | |||
| } | |||
| } | |||
✅ Deploy Preview for zio-http ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@gipeshka Please fix the build. Rebase might just do it |
348ecb6 to
dfcbdf6
Compare
|
@987Nabil thanks for letting me know, it's fixed now |
987Nabil
left a comment
There was a problem hiding this comment.
Found a functional bug in the zstd defaults — see inline comment. The blockSize/maxEncodeSize defaults are swapped relative to Netty's actual ZstdConstants. With the current values, CompressionOptions.zstd() (no args) sets maxEncodeSize = 65536, so any response body that compresses to more than 64 KB will throw (maxEncodeSize bounds the largest compressible object), while blockSize = Integer.MAX_VALUE is backwards too. Everything else (Conversions.scala mapping, param order into StandardCompressionOptions.zstd(level, blockSize, maxEncodeSize), config parsing wiring, CI/Mima) checks out — this is the one blocking issue.
19bf52c to
31d500d
Compare
| typ.toLowerCase match { | ||
| case "gzip" => gzip(level, bits, mem) | ||
| case "deflate" => deflate(level, bits, mem) | ||
| case "brotli" => brotli(quantity, lgwin, mode) | ||
| case "zstd" => zstd(level, block, maxencode) |
There was a problem hiding this comment.
makes sense. @987Nabil what would be the preferred way of solving it? Would isolating level for zstd under a dedicated config prefix like zstdlevel work?
There was a problem hiding this comment.
@987Nabil I've adjusted it via optional. Please have a look
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
zio-http/shared/src/main/scala/zio/http/Server.scala:422
- All sibling public compression factories document that their defaults correspond to Netty's default option object, but the newly added zstd factory omits that API documentation. Document the same default relationship here so callers can understand the three otherwise non-obvious tuning parameters.
def zstd(
zio-http/shared/src/main/scala/zio/http/Server.scala:321
- The new supported encoding is not represented by the typed header API.
Header.AcceptEncodingstill exposes zstd only asUnknown, andHeader.ContentEncoding.parsedropszstd, so a response compressed by this option cannot be observed throughresponse.header(Header.ContentEncoding). Add Zstd variants to both header algebras, update their parsers/rendering, and cover them inHeaderSpec.
final case class Zstd(cfg: ZstdConfig) extends CompressionOptions { val name = "zstd" }
zio-http/jvm/src/main/scala/zio/http/netty/model/Conversions.scala:174
- This new conversion branch has no end-to-end coverage, although
ResponseCompressionSpecverifies the existing response codecs. Add a test-scoped zstd-jni dependency and an integration test that enables only this option, requestsAccept-Encoding: zstd, verifiesContent-Encoding: zstd, and decompresses the body; a test-scoped dependency would not impose JNI on library consumers.
case CompressionOptions.Zstd(cfg) =>
StandardCompressionOptions.zstd(cfg.level, cfg.blockSize, cfg.maxEncodeSize)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
141aaa5 to
fdbbd4c
Compare
Addressing #4184.
Not including zstd jni, to use this I assume one would have to manually enable it (just like one has to do it now for zio-http to start accepting zstd
content-encoding)