vy commented on code in PR #4071:
URL: https://github.com/apache/logging-log4j2/pull/4071#discussion_r3159880376
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/GzCompressAction.java:
##########
@@ -64,26 +69,45 @@ public final class GzCompressAction extends AbstractAction {
* does not cause an exception to be thrown or affect
return value.
* @param compressionLevel
* Gzip deflater compression level.
+ * @param maxDelaySeconds
+ * Maximum delay in seconds before compression.
*/
public GzCompressAction(
- final File source, final File destination, final boolean
deleteSource, final int compressionLevel) {
+ final File source,
+ final File destination,
+ final boolean deleteSource,
+ final int compressionLevel,
+ final int maxDelaySeconds) {
Objects.requireNonNull(source, "source");
Objects.requireNonNull(destination, "destination");
this.source = source;
this.destination = destination;
this.deleteSource = deleteSource;
this.compressionLevel = compressionLevel;
+ this.maxDelaySeconds = maxDelaySeconds;
+ }
+
+ /**
+ * Legacy constructor for backward compatibility.
Review Comment:
No need to call it legacy, it is still legitimate.
```suggestion
* Creates a new instance.
*
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/GzCompressAction.java:
##########
@@ -94,6 +118,16 @@ public GzCompressAction(final File source, final File
destination, final boolean
*/
@Override
public boolean execute() throws IOException {
+ if (maxDelaySeconds > 0) {
+ int delay =
java.util.concurrent.ThreadLocalRandom.current().nextInt(maxDelaySeconds + 1);
+ if (delay > 0) {
+ try {
+ Thread.sleep(delay * 1000L);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ }
Review Comment:
Can you move this to a new (package-private!) `static void blockThread(int
maxDelaySeconds)` method in `AbstractAction`, please?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/GzCompressAction.java:
##########
@@ -64,26 +69,45 @@ public final class GzCompressAction extends AbstractAction {
* does not cause an exception to be thrown or affect
return value.
* @param compressionLevel
* Gzip deflater compression level.
+ * @param maxDelaySeconds
+ * Maximum delay in seconds before compression.
*/
Review Comment:
Can you add a `@since 2.26.0`?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ZipCompressAction.java:
##########
@@ -59,15 +64,35 @@ public final class ZipCompressAction extends AbstractAction
{
* @param deleteSource if true, attempt to delete file on completion.
Failure to delete does not cause an exception
* to be thrown or affect return value.
* @param level TODO
+ * @param maxDelaySeconds maximum delay in seconds before compression.
*/
- public ZipCompressAction(final File source, final File destination, final
boolean deleteSource, final int level) {
+ public ZipCompressAction(
+ final File source,
+ final File destination,
+ final boolean deleteSource,
+ final int level,
+ final int maxDelaySeconds) {
Objects.requireNonNull(source, "source");
Objects.requireNonNull(destination, "destination");
this.source = source;
this.destination = destination;
this.deleteSource = deleteSource;
this.level = level;
+ this.maxDelaySeconds = maxDelaySeconds;
+ }
+
+ /**
+ * Creates new instance of GzCompressAction with default maxDelaySeconds.
Review Comment:
```suggestion
* Creates new instance.
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ZipCompressAction.java:
##########
@@ -59,15 +64,35 @@ public final class ZipCompressAction extends AbstractAction
{
* @param deleteSource if true, attempt to delete file on completion.
Failure to delete does not cause an exception
* to be thrown or affect return value.
* @param level TODO
Review Comment:
Can you replace all `@param level TODO` with `@param level the compression
level`, please?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ZipCompressAction.java:
##########
@@ -59,15 +64,35 @@ public final class ZipCompressAction extends AbstractAction
{
* @param deleteSource if true, attempt to delete file on completion.
Failure to delete does not cause an exception
* to be thrown or affect return value.
* @param level TODO
+ * @param maxDelaySeconds maximum delay in seconds before compression.
*/
- public ZipCompressAction(final File source, final File destination, final
boolean deleteSource, final int level) {
+ public ZipCompressAction(
+ final File source,
+ final File destination,
+ final boolean deleteSource,
+ final int level,
Review Comment:
`ZipOutputStream#setLevel(int)` accepts values in the `[0,9]` range, throws
`IAE` otherwise. I think we should perform this validation in ctors here too.
That is, create a `checkLevel(int)` method throwing `IAE` on illegal input, and
use it in ctors.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java:
##########
@@ -401,7 +429,38 @@ public static DefaultRolloverStrategy createStrategy(
.setStopCustomActionsOnError(stopCustomActionsOnError)
.setConfig(config)
.build();
- // @formatter:on
+ }
+
+ /**
+ * New factory method supporting maxCompressionDelaySeconds.
+ * @since 2.26.0
+ */
+ @PluginFactory
+ public static DefaultRolloverStrategy createStrategy(
Review Comment:
This is not needed. Shouldn't the users be using the builder API instead?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java:
##########
@@ -477,6 +538,30 @@ protected DefaultRolloverStrategy(
this.customActions = customActions == null ?
Collections.<Action>emptyList() : Arrays.asList(customActions);
this.tempCompressedFilePattern =
tempCompressedFilePatternString != null ? new
PatternProcessor(tempCompressedFilePatternString) : null;
+ this.maxCompressionDelaySeconds = 0; // Default for backward
compatibility
+ }
+
+ // Overloaded constructor for new feature
+ protected DefaultRolloverStrategy(
Review Comment:
You have multiple ctors with cloned logic:
```
Foo(int a, int b) {
this.a = a;
this.b = b;
this.c = 0;
}
Foo(int a, int b, int c) {
this.a = a;
this.b = b;
this.c = c;
}
```
Instead have one canonical ctor and make all other ctors defer to that:
```
Foo(int a, int b) {
this(a, b, 0);
}
Foo(int a, int b, int c) {
this.a = a;
this.b = b;
this.c = c;
}
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/FileExtension.java:
##########
@@ -45,7 +62,24 @@ public Action createCompressAction(
final String compressedName,
final boolean deleteSource,
final int compressionLevel) {
- return new GzCompressAction(source(renameTo),
target(compressedName), deleteSource, compressionLevel);
+ return new GzCompressAction(
+ new File(renameTo), new File(compressedName),
deleteSource, compressionLevel, 0);
+ }
+
+ @Override
+ public Action createCompressAction(
+ final String renameTo,
+ final String compressedName,
+ final boolean deleteSource,
+ final int compressionLevel,
+ final int maxCompressionDelaySeconds) {
+ // Issue #4012: pass maxCompressionDelaySeconds so random delay is
applied before compression
Review Comment:
```suggestion
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/FileExtension.java:
##########
@@ -129,9 +163,21 @@ public static FileExtension lookupForFile(final String
fileName) {
this.extension = extension;
}
+ // 4-argument version (legacy, for compatibility)
public abstract Action createCompressAction(
String renameTo, String compressedName, boolean deleteSource, int
compressionLevel);
+ // 5-argument version (for delay support)
+ public Action createCompressAction(
+ String renameTo,
+ String compressedName,
+ boolean deleteSource,
+ int compressionLevel,
+ int maxCompressionDelaySeconds) {
+ // By default, ignore delay and call the 4-argument version
+ return createCompressAction(renameTo, compressedName, deleteSource,
compressionLevel);
Review Comment:
Huh? Why do we discard the `maxCompressionDelaySeconds` here?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/FileExtension.java:
##########
@@ -129,9 +163,21 @@ public static FileExtension lookupForFile(final String
fileName) {
this.extension = extension;
}
+ // 4-argument version (legacy, for compatibility)
public abstract Action createCompressAction(
String renameTo, String compressedName, boolean deleteSource, int
compressionLevel);
+ // 5-argument version (for delay support)
Review Comment:
```suggestion
```
##########
src/changelog/.2.x.x/plugin_processor_min_allowed_message_kind.xml:
##########
Review Comment:
Revert these changes
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java:
##########
@@ -148,6 +151,19 @@ public DefaultRolloverStrategy build() {
// The config object can be null when this object is built
programmatically.
final StrSubstitutor nonNullStrSubstitutor =
config != null ? config.getStrSubstitutor() : new
StrSubstitutor();
+ // Legacy constructor for backward compatibility
+ if (maxCompressionDelaySeconds == 0) {
+ return new DefaultRolloverStrategy(
+ minIndex,
+ maxIndex,
+ useMax,
+ compressionLevel,
+ nonNullStrSubstitutor,
+ customActions,
+ stopCustomActionsOnError,
+ tempCompressedFilePattern);
+ }
+ // New constructor with delay
Review Comment:
Can't we simply remove this block?
```suggestion
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/FileExtension.java:
##########
@@ -35,7 +35,24 @@ public Action createCompressAction(
final String compressedName,
final boolean deleteSource,
final int compressionLevel) {
- return new ZipCompressAction(source(renameTo),
target(compressedName), deleteSource, compressionLevel);
+ return new ZipCompressAction(
+ new File(renameTo), new File(compressedName),
deleteSource, compressionLevel, 0);
+ }
+
+ @Override
+ public Action createCompressAction(
+ final String renameTo,
+ final String compressedName,
+ final boolean deleteSource,
+ final int compressionLevel,
+ final int maxCompressionDelaySeconds) {
+ // Issue #4012: pass maxCompressionDelaySeconds so random delay is
applied before compression
Review Comment:
```suggestion
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java:
##########
@@ -380,10 +402,16 @@ public static Builder newBuilder() {
* @return A DefaultRolloverStrategy.
* @deprecated Since 2.9 Usage of Builder API is preferable
*/
+ // ...existing code...
+ // ...existing code...
Review Comment:
@ramanathan1504, please don't leave any LLM leftovers.
```suggestion
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java:
##########
@@ -680,11 +769,19 @@ public RolloverDescription rollover(final
RollingFileManager manager) throws Sec
}
compressAction = new CompositeAction(
Arrays.asList(
- fileExtension.createCompressAction(renameTo,
tmpCompressedName, true, compressionLevel),
+ ((FileExtension) fileExtension)
Review Comment:
Is this casting (and the one down below) needed?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java:
##########
Review Comment:
Revert these changes
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/FileExtension.java:
##########
@@ -129,9 +163,21 @@ public static FileExtension lookupForFile(final String
fileName) {
this.extension = extension;
}
+ // 4-argument version (legacy, for compatibility)
Review Comment:
```suggestion
```
##########
src/site/antora/modules/ROOT/pages/manual/plugins.adoc:
##########
Review Comment:
Revert these changes
--
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]
