ramanathan1504 commented on code in PR #4226:
URL: https://github.com/apache/logging-log4j2/pull/4226#discussion_r3851394450
##########
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:
`testBuilderWithoutFileNameInitializesPromptly` passes with either fix alone
(0.134 s / 0.143 s) and fails only when both are reverted, so nothing pins this
guard — is it load-bearing, or documentation like
`TimeBasedTriggeringPolicy.initialize()`'s `getFileTime() == 0`?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/util/CronExpression.java:
##########
@@ -1581,6 +1581,13 @@ protected Date getTimeBefore(final Date targetDate) {
Date prevFireTime;
do {
final Date prevCheckDate = new Date(start.getTime() -
minIncrement);
+ // `getTimeAfter()` never returns a fire time before 1970, so once
the candidate
+ // date precedes `MIN_DATE` the loop condition below can no longer
be satisfied.
+ // Bail out here, otherwise the search walks back millennia one
increment at a
+ // time before `getTimeAfter()` finally gives up at the calendar's
upper bound.
+ if (prevCheckDate.before(MIN_DATE)) {
+ return null;
+ }
Review Comment:
`MIN_CAL.set(1970, 0, 1)` never clears the time fields, so `MIN_DATE`
carries the JVM's start time of day — I printed `12:01:08.151`, `12:03:05.302`
and `12:01:39.611` across three runs.
That only mattered once this guard started bounding the *candidate*: with `0
0 0 * * ?`, `getPrevFireTime(1970-01-02 06:00)` gives `1970-01-02 00:00:00` on
2.x and `null` here. Bounding on the epoch is deterministic — verified green
plus spotless.
````suggestion
// `getTimeAfter()` never returns a fire time before 1970, so
once the candidate
// date precedes the epoch the loop condition below can no
longer be satisfied.
// Bail out here, otherwise the search walks back millennia one
increment at a
// time before `getTimeAfter()` finally gives up at the
calendar's upper bound.
if (prevCheckDate.getTime() < 0) {
return null;
}
````
--
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]
