[katstack]

This PR adds ZstdCompressAction, replacing the generic CommonsCompressAction("zstd", ...) path (added in #1514) with a dedicated action that supports a configurable compressionLevel, following the same incremental pattern already used by GzCompressAction (LOG4J2-2598) and ZipCompressAction.

Supported range is the standard positive Zstd levels [1, ZstdConstants.ZSTD_CLEVEL_MAX] (currently [1, 22]). compressionLevel=-1 (unset) continues to resolve to the Zstd default level (3), consistent with the existing behavior documented in FileExtension/rolling-file.adoc.

What this PR deliberately does not do

Negative ("fast compression") Zstd levels are not supported. The rolling-appender framework uses Deflater.DEFAULT_COMPRESSION (-1) as a format-agnostic sentinel for "unspecified compression level," so an explicit compressionLevel=-1 would be indistinguishable from "not set." On main, compressionLevel has already been replaced by a more generic compressionOptions map (#2921); on 2.x it remains the mechanism, so this PR stays scoped to extending it rather than adding another special case on top of the sentinel.

Testing

ZstdCompressActionTest covers the new validation/mapping logic, including a test pinning the currently-assumed ZstdConstants bounds so a future zstd-jni/commons-compress upgrade that shifts them fails loudly instead of silently drifting from the documented range. Also ran the existing GzCompressActionTest/ZipCompressActionTest/Bzip2CompressActionTest/rolling-appender suites locally with no regressions, and ./mvnw verify passes on both JDK 8 and JDK 17.

References

• Configurable compression algorithms #2950 — ongoing discussion on generalizing compression configuration.

[ramanathan1504]

Thanks @katstack, the write-up on why -1 has to stay the zstd default is clear, and the FileExtension mapping has a test.
FileExtension.ZSTD now loads ZstdConstants and the constructor checks the range while the rollover is being built, so compressionLevel="0" or a missing zstd-jni jar stops the rollover and every later log event is dropped. Can the level check stay inside execute(), so those cases only fail the compression like on 2.x, and can you add a src/changelog/.2.x.x entry?

[ramanathan1504]

• For @ppkarwasz / @vy: given Configurable compression algorithms #2950 proposed deprecating compressionLevel, is new 2.x-only public API (ZstdCompressAction) wanted, or should the manual just keep .zst as "not supported"?
• Should testAssumedZstdLevelBoundsHaveNotChanged stay, knowing a zstd-jni bump can turn it red?
• Separate issue: should RollingFileManager still write the event when strategy.rollover throws? Today a bad .gz level loses events the same way.

[katstack]

Addressed in 892c912. FileExtension.ZSTD now passes the raw level without loading ZstdConstants; ZstdCompressAction.execute() resolves the unspecified -1 level and validates it immediately before compression. Therefore an invalid level or missing zstd-jni only fails the asynchronous compression action, rather than preventing rollover. I also added the 2.x changelog entry, removed the @apiNote, and added @since 2.27.0.

[ramanathan1504]

@ppkarwasz mvnw verify is red on log4j-osgi-test: ZstdCompressorOutputStream.builder() pulls in org.apache.commons.io.build and bnd makes it a mandatory import, so the
log4j-core bundle stops resolving for everyone, zstd users or not. Pushed org.apache.commons.io.*;resolution:=optional into log4j-core/pom.xml next to the
commons-compress entry, and Equinox and Felix are 9/9 again.
@ppkarwasz, one thing before this goes further: #2950 proposed deprecating compressionLevel and removing it in 3.x, so is a new 2.x-only ZstdCompressAction wanted at all?

[ppkarwasz]

Hi @katstack,

Thank you for the PR.

As @ramanathan1504 mentioned, we are considering removing compressionLevel in 3.x in favor of a more general mechanism (#2950): every compression algorithm has its own set of parameters, and adding a RolloverStrategy attribute for each of them does not scale. Could you have a look at #2950 and tell us about your use case? Your point of view would help us shape that design.

That said, the attribute exists today, so we might as well honor it wherever we can. ZStandard is not the only algorithm with a compression level: Deflate, GZip and BZip2 support one too. We prefer to change a whole class of implementations at once rather than a single one, so users get consistent behavior and the change can be properly described in the release notes. Would you be willing to extend the PR to the other three algorithms?

Two remarks on the implementation:

• Please move ZstdCompressAction to a new actions.internal package, so it is not exported via JPMS/OSGi. Everything we export becomes public API that we must keep compatible for years, so we only export components meant to be reused. These compression actions are Log4j-specific; reusable compression code belongs in Commons Compress.
• CommonsCompressAction, GzCompressAction and ZipCompressAction already duplicate a lot of logic. Extracting a common AbstractCompressAction (also in actions.internal) would let the new action share it instead of adding a fourth copy.

TL;DR: moving ZstdCompressAction to actions.internal is the one required change; covering the other compressionLevel-aware algorithms in this PR would be very welcome.

[katstack]

Hi @ppkarwasz, thank you for the detailed feedback. 

Our use case is an air-gapped bare-metal environment where local disk capacity is constrained, while CPU headroom is available during rollover. The customer requirement is to retain application logs locally for as long as possible, so trading additional rollover CPU time for a better compression ratio is valuable. 

For a representative, highly repetitive application-log workload, we observed that Zstd level 9 reduced the archive to roughly 1% of the original size. This is workload-specific rather than a general Zstd result, but it illustrates why operators need to tune the CPU/compression-ratio trade-off instead of always using a fixed default. 

I will move ZstdCompressAction to actions.internal, then update the PR to provide consistent compressionLevel support for Deflate, GZip, and BZip2. I will also extract the shared compression logic into an internal base class. 

[ramanathan1504]

Thanks, this is a much better shape: one copy loop in CompressActionSupport, the action out of the exported package, and the new RollingFileManagerTest case pins that a bad level only fails the async compression.
That case is parameterized over bz2 and deflate only. Adding gz and zip to it fails, because GzCompressAction and ZipCompressAction still check the level in their constructors, which run inside the rollover, so those two still lose the log event. Could they move onto AbstractCompressAction too, and could you merge current 2.x so log4j-api-test goes green again?

[katstack]

@ramanathan1504 Before I make another code change, could we agree on the semantics for unsupported compressionLevel values?

Format	Accepted configured levels	Effective default for -1	Meaning of 0	Behavior before this branch	Proposed handling of unsupported values
ZIP	-1, 0..9	JDK Deflater default	No compression	Valid levels were applied. Unsupported values were rejected while creating the rollover action, which could lose the triggering event.	Fall back to the default and log a warning asynchronously.
GZip	-1, 0..9	JDK Deflater default	No compression	Valid levels were applied. Unsupported values were rejected while creating the rollover action, which could lose the triggering event.	Fall back to the default and log a warning asynchronously.
BZip2	-1, 1..9	9	Unsupported	compressionLevel was ignored; compression always used the Commons Compress default level 9.	Fall back to 9 and log a warning asynchronously.
Deflate	-1, 0..9	JDK Deflater default	No compression	compressionLevel was ignored; compression always used the Commons Compress / JDK default.	Fall back to the default and log a warning asynchronously.
Zstd	-1, 0, 1..ZSTD_CLEVEL_MAX	ZSTD_CLEVEL_DEFAULT (currently 3)	Zstd default	compressionLevel was ignored; compression always used the Commons Compress / Zstd default.	Fall back to the default and log a warning asynchronously.

I would prefer to avoid format-specific coercions. -1 would remain the Log4j sentinel for selecting the format-specific default, and supported values would be passed through unchanged.

Zstd would be the only explicit mapping: its native -1 represents a fast-compression level, while Log4j uses -1 for an unspecified level, so Log4j's -1 needs to map to the Zstd default. Other negative Zstd fast-compression levels would remain unsupported for now and use the fallback behavior above.

With this approach, BZip2 level 0 would not be coerced to level 1; it would be treated as unsupported and fall back to BZip2's default level (9).

Does that match the intended contract for the shared compressionLevel attribute?

[ramanathan1504]

That contract looks right, and it matches DefaultRolloverStrategy already: a min below 1 or a max below min logs and falls back instead of failing. It also removes the lost event on .gz and .zip.
It flips the tests that assert IllegalArgumentException, and a warning per rollover repeats forever, so log it once per resolved level. @ppkarwasz, this changes documented .zip and .gz behaviour, so your call before @katstack writes it.

[katstack]

Hi @ppkarwasz, could you share your thoughts on the proposed fallback behavior for unsupported compression levels? Is there anything else you’d like me to clarify before I proceed with the implementation? Thanks!

[ramanathan1504]

@katstack let's go with your table. That contract is fine and it matches what DefaultRolloverStrategy already does with min and max: log and fall back rather than fail. Unsupported level means the format default plus one warning per resolved level, -1 stays the sentinel, and .bz2 0 falls back to 9 with no coercion rule of its own.
In the same pass please move GzCompressAction and ZipCompressAction onto AbstractCompressAction so no constructor throws during rollover, flip the tests that assert IllegalArgumentException, and add a one-line fixed changelog entry for the lost rollover event. @ppkarwasz, say so if you want it differently, otherwise this is the direction.

[ppkarwasz]

Hi @ppkarwasz, could you share your thoughts on the proposed fallback behavior for unsupported compression levels? Is there anything else you’d like me to clarify before I proceed with the implementation? Thanks!

Regarding the fallback behavior, it looks like a good idea, although we could probably validate the value sooner, when RolloverStrategy is instantiated and warn the user only once. For example we could expose resolveCompressionLevel in FileExtension

Regarding the mapping of -1 and 0: I don't see any need to provide a coherent set of compression levels. Let's just use the native levels of each compressor (which for zstd will allow for negative values too). If the user does not specify a compression level explicitly, we could use some marker value that is not a valid compression level for any algorithm. Integer.MIN_VALUE seems like a good candidate.

[katstack]

Hi @ppkarwasz, could you share your thoughts on the proposed fallback behavior for unsupported compression levels? Is there anything else you’d like me to clarify before I proceed with the implementation? Thanks!

Regarding the fallback behavior, it looks like a good idea, although we could probably validate the value sooner, when RolloverStrategy is instantiated and warn the user only once. For example we could expose resolveCompressionLevel in FileExtension

Regarding the mapping of -1 and 0: I don't see any need to provide a coherent set of compression levels. Let's just use the native levels of each compressor (which for zstd will allow for negative values too). If the user does not specify a compression level explicitly, we could use some marker value that is not a valid compression level for any algorithm. Integer.MIN_VALUE seems like a good candidate.

Using Integer.MIN_VALUE to distinguish an unspecified level makes sense to me. I've applied that along with the other changes you suggested.

[ppkarwasz]

@katstack,

I would still move all the implementations of AbstractCompressAction into the internal package and replace the public ones with shims that reuse the internal classes.

Otherwise this looks good to merge.

[katstack]

@ppkarwasz, done. The implementations are in action.internal now, and the public GzCompressAction and ZipCompressAction are deprecated shims that delegate to them.

[ppkarwasz]

@katstack,

Looks good to me, thanks!

[ramanathan1504]

The eager resolution caches per format so the warning lands once, the fallback keeps the rollover alive for every unsupported level, and the deprecated shims leave the exported surface unchanged against 2.26.1.
Two changelog entries run to two sentences; one line each reads better, and main already has rolling/action/internal, so this lines the two branches up. Approving with that nit.

[katstack]

Applied the changelog suggestions from @ramanathan1504. @ppkarwasz, could you approve the workflows and merge once CI is green? Thanks!

[ramanathan1504]

LGTM!