tupelo-schneck commented on code in PR #4226:
URL: https://github.com/apache/logging-log4j2/pull/4226#discussion_r3853938753
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java:
##########
@@ -64,7 +64,11 @@ private CronTriggeringPolicy(
public void initialize(final RollingFileManager aManager) {
this.manager = aManager;
final Date now = new Date();
- final Date lastRollForFile = cronExpression.getPrevFireTime(new
Date(this.manager.getFileTime()));
+ // `RollingFileManager` reports a file time of 0 when there is no
current file yet, which
+ // happens on every startup of an appender configured without a
`fileName`. There is no
+ // previous roll to look up in that case, so skip the lookup entirely.
+ final long fileTime = this.manager.getFileTime();
Review Comment:
Documentation, not load-bearing — your second reading is right, and thank
you for checking it rather than assuming.
I confirmed it: with the epoch bound in `CronExpression` in place and this
guard reverted, `CronTriggeringPolicyTest` and `CronExpressionTest` pass in
0.18 s. Once the search is bounded, `getPrevFireTime(new Date(0))` returns
`null` in microseconds for any expression, so nothing here depends on the guard
for speed.
I have kept it, for the reason you name: it mirrors
`TimeBasedTriggeringPolicy.initialize()`'s `getFileTime() == 0` check, and it
makes `lastRollForFile` `null` by construction rather than by a lookup that
happens to return `null` for a meaningless input. Happy to drop it if you would
rather keep the diff minimal — no functional difference either way.
--
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]
