ramanathan1504 commented on code in PR #4234:
URL: https://github.com/apache/logging-log4j2/pull/4234#discussion_r4030108706
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/FileExtension.java:
##########
@@ -99,8 +101,15 @@ public Action createCompressAction(
final String compressedName,
final boolean deleteSource,
final int compressionLevel) {
- // One of "gz", "bzip2", "xz", "zstd", "pack200", or "deflate".
- return new CommonsCompressAction("zstd", source(renameTo),
target(compressedName), deleteSource);
+ // -1 (Deflater.DEFAULT_COMPRESSION) is the framework-wide
sentinel for 'unspecified compression level'.
+ // Unlike GZ/ZIP, where -1 has no meaning other than 'use
Deflater's default' (java.util.zip.Deflater
+ // natively treats -1 as its own default-compression sentinel),
Zstd defines -1 as a real, distinct
+ // fast-compression level. So the sentinel has to be mapped
explicitly here to Zstd's own default level.
+ // Negative Zstd fast-compression levels are intentionally out of
scope for now; see the
+ // discussion on generalizing compressionLevel at
+ // https://github.com/apache/logging-log4j2/discussions/2950.
+ final int level = compressionLevel == -1 ?
ZstdConstants.ZSTD_CLEVEL_DEFAULT : compressionLevel;
Review Comment:
`ZstdConstants` reads its values from zstd-jni when the class loads. Here
that happens during rollover, so without the zstd-jni jar this throws
`NoClassDefFoundError` before the file is renamed. Can the `-1` mapping move
into `ZstdCompressAction.execute`?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ZstdCompressAction.java:
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.logging.log4j.core.appender.rolling.action;
+
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.util.Objects;
+import
org.apache.commons.compress.compressors.zstandard.ZstdCompressorOutputStream;
+import org.apache.commons.compress.compressors.zstandard.ZstdConstants;
+
+/**
+ * Compresses a file using Zstandard compression.
+ * <p>
+ * Supports positive compression levels in the range [{@value
#MIN_COMPRESSION_LEVEL}, {@link ZstdConstants#ZSTD_CLEVEL_MAX}].
+ * Negative (fast-compression) levels are not currently supported; this may
change in a future release.
+ * </p>
+ *
+ * @apiNote An explicitly configured level of -1 currently resolves to the
Zstd default level (3).
Review Comment:
The constructor throws for `-1`. Only `FileExtension.ZSTD` turns it into 3.
Can this note go, since the manual already covers it?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ZstdCompressAction.java:
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.logging.log4j.core.appender.rolling.action;
+
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.util.Objects;
+import
org.apache.commons.compress.compressors.zstandard.ZstdCompressorOutputStream;
+import org.apache.commons.compress.compressors.zstandard.ZstdConstants;
+
+/**
+ * Compresses a file using Zstandard compression.
+ * <p>
+ * Supports positive compression levels in the range [{@value
#MIN_COMPRESSION_LEVEL}, {@link ZstdConstants#ZSTD_CLEVEL_MAX}].
+ * Negative (fast-compression) levels are not currently supported; this may
change in a future release.
+ * </p>
+ *
+ * @apiNote An explicitly configured level of -1 currently resolves to the
Zstd default level (3).
+ * This is provisional behavior tied to the current lack of negative-level
support and may change
+ * in a future release without a corresponding API signature change.
+ */
Review Comment:
New public class on `2.x`, so it needs the version tag.
```suggestion
* @since 2.27.0
*/
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ZstdCompressAction.java:
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.logging.log4j.core.appender.rolling.action;
+
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.util.Objects;
+import
org.apache.commons.compress.compressors.zstandard.ZstdCompressorOutputStream;
+import org.apache.commons.compress.compressors.zstandard.ZstdConstants;
+
+/**
+ * Compresses a file using Zstandard compression.
+ * <p>
+ * Supports positive compression levels in the range [{@value
#MIN_COMPRESSION_LEVEL}, {@link ZstdConstants#ZSTD_CLEVEL_MAX}].
+ * Negative (fast-compression) levels are not currently supported; this may
change in a future release.
+ * </p>
+ *
+ * @apiNote An explicitly configured level of -1 currently resolves to the
Zstd default level (3).
+ * This is provisional behavior tied to the current lack of negative-level
support and may change
+ * in a future release without a corresponding API signature change.
+ */
+public final class ZstdCompressAction extends AbstractAction {
+
+ private static final int BUF_SIZE = 8192;
+
+ /**
+ * Minimum supported Zstd compression level. Negative (fast-compression)
levels are intentionally
+ * out of scope for now; see the discussion at
+ * https://github.com/apache/logging-log4j2/discussions/2950.
+ */
+ static final int MIN_COMPRESSION_LEVEL = 1;
+
+ /**
+ * Source file.
+ */
+ private final File source;
+
+ /**
+ * Destination file.
+ */
+ private final File destination;
+
+ /**
+ * If true, attempt to delete file on completion.
+ */
+ private final boolean deleteSource;
+
+ /**
+ * Zstandard compression level to use.
+ *
+ * @see ZstdCompressorOutputStream.Builder#setLevel(int)
+ */
+ private final int compressionLevel;
+
+ /**
+ * Validates that the compression level is a positive integer in the range
[{@value #MIN_COMPRESSION_LEVEL}, {@link ZstdConstants#ZSTD_CLEVEL_MAX}].
+ *
+ * @param compressionLevel Zstandard compression level
+ * @return the compression level if valid
+ * @throws IllegalArgumentException if compressionLevel is not in the
range [{@value #MIN_COMPRESSION_LEVEL}, {@link ZstdConstants#ZSTD_CLEVEL_MAX}]
+ */
+ private static int checkCompressionLevel(final int compressionLevel) {
+ final int minCompressionLevel = MIN_COMPRESSION_LEVEL;
+ final int maxCompressionLevel = ZstdConstants.ZSTD_CLEVEL_MAX;
+
+ if (compressionLevel < minCompressionLevel || compressionLevel >
maxCompressionLevel) {
+ if (compressionLevel < 0) {
+ throw new IllegalArgumentException(
+ "Negative Zstd fast-compression levels are not yet
supported by Log4j2 (got: "
+ + compressionLevel
+ + "). Only the standard range ["
+ + minCompressionLevel
+ + ", "
+ + maxCompressionLevel
+ + "] is currently supported.");
+ }
+ throw new IllegalArgumentException("Zstd compression level must be
in the range ["
+ + minCompressionLevel
+ + ", "
+ + maxCompressionLevel
+ + "], got: "
+ + compressionLevel);
+ }
+ return compressionLevel;
+ }
+
+ /**
+ * Creates a new instance.
+ *
+ * @param source file to compress, may not be null.
+ * @param destination compressed file, may not be null.
+ * @param deleteSource if true, attempt to delete file on completion.
+ * @param compressionLevel Zstandard compression level.
+ */
+ public ZstdCompressAction(
+ final File source, final File destination, final boolean
deleteSource, final int compressionLevel) {
+ Objects.requireNonNull(source, "source");
+ Objects.requireNonNull(destination, "destination");
+
+ this.source = source;
+ this.destination = destination;
+ this.deleteSource = deleteSource;
+ this.compressionLevel = checkCompressionLevel(compressionLevel);
Review Comment:
This throws `IllegalArgumentException` inside
`DefaultRolloverStrategy.rollover`, where nothing catches it. With
`compressionLevel="0"` on a `.zst` pattern the file stops rolling and the log
events are lost. `0` was ignored for `.zst` before. Can the check run in
`execute()` instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
