Skip to content

Commit

Permalink
Issue #1051 - Using java.time.ZonedDateTime instead of java.util.Cale…
Browse files Browse the repository at this point in the history 
…ndar for Java 1.8+
  • Loading branch information
@joakime
joakime committed Nov 17, 2016
1 parent 7930a3d commit 9f317de
Showing 1 changed file with 9 additions and 14 deletions.
23 changes: 9 additions & 14 deletions jetty-util/src/main/java/org/eclipse/jetty/util/RolloverFileOutputStream.java
View file
Expand Up @@ -24,7 +24,8 @@
import java.io.IOException;
import java.io.OutputStream;
import java.text.SimpleDateFormat;
import java.util.Calendar;
import java.time.ZonedDateTime;
import java.time.temporal.ChronoUnit;
import java.util.Date;
import java.util.Locale;
import java.util.TimeZone;
Expand Down Expand Up @@ -53,7 +54,7 @@ public class RolloverFileOutputStream extends FilterOutputStream
final static int ROLLOVER_FILE_RETAIN_DAYS = 31;

private RollTask _rollTask;
private Calendar midnight;
private ZonedDateTime midnight;
private SimpleDateFormat _fileBackupFormat;
private SimpleDateFormat _fileDateFormat;

Expand Down Expand Up @@ -175,13 +176,7 @@ public RolloverFileOutputStream(String filename,

_rollTask=new RollTask();

midnight = Calendar.getInstance();
midnight.setTimeZone(zone);
// set to midnight
midnight.set(Calendar.HOUR, 0);
midnight.set(Calendar.MINUTE, 0);
midnight.set(Calendar.SECOND, 0);
midnight.set(Calendar.MILLISECOND, 0);
midnight = ZonedDateTime.now().toLocalDate().atStartOfDay(zone.toZoneId());

scheduleNextRollover();
}
Expand All @@ -193,8 +188,8 @@ private void scheduleNextRollover()
// Using Calendar.add(DAY, 1) takes in account Daylights Savings
// differences, and still maintains the "midnight" settings for
// Hour, Minute, Second, Milliseconds
midnight.add(Calendar.DAY_OF_MONTH, 1);
__rollover.schedule(_rollTask,midnight.getTime());
midnight = midnight.toLocalDate().plus(1, ChronoUnit.DAYS).atStartOfDay(midnight.getZone());
__rollover.schedule(_rollTask,midnight.toInstant().toEpochMilli());

This comment has been minimized.

Sign in to view

Copy link
@panchi

panchi Feb 26, 2017

Hi, it seems that using schedule() with Date as it previously was is not the same as using with long as is now, as the long parameter is a delay:
https://docs.oracle.com/javase/7/docs/api/java/util/Timer.html#schedule(java.util.TimerTask,%20java.util.Date)
vs
https://docs.oracle.com/javase/7/docs/api/java/util/Timer.html#schedule(java.util.TimerTask,%20long)

This results in no rollout performed the next midnight.

¿Would it make sense to wrap the second argument with new Date(..)?

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Feb 27, 2017

Author Contributor

If it doesn't trigger in your case, then its something else causing it.
The schedule(task, long) is fully supported by the Timer class.

Using time in milliseconds eliminates the faults that can be introduced in scheduled Timer tasks based on Date (usually seen from system changes: updates in tz data, ntp updates, etc)

The ZonedDateTime math (then conversion to millis) means the scheduled task will run, regardless of the changes to date/time that occur elsewhere on your system.

If this is a problem on your system, then consider using a formal logging framework (slf4j -> logback, or slf4j -> log4j) and set up its rolling file loggers.

This class, RolloverFileOutputStream, exists for etc/jetty-logging.xml to capture STDOUT and STDERR and write a log file.

This comment has been minimized.

Sign in to view

Copy link
@panchi

panchi Feb 27, 2017 •
edited

Hi Joakime, the documentation states that the long parameter is a delay parameter rather than an epoch-milli-timestamp.

Moreover, if you look at that method source you will se that the delay parameter is added to System.currentTimeMills.

I think, the schedule method which receives a Date parameter must be used as it was before. The code may remain the same as it is now, and the second parameter of the schedule call wrapped with a new Date(...)
}

/* ------------------------------------------------------------ */
Expand Down Expand Up @@ -265,9 +260,9 @@ private void removeOldFiles()
{
if (_retainDays>0)
{
Calendar now = Calendar.getInstance();
now.add(Calendar.DAY_OF_MONTH, (-1)*_retainDays);
long expired = now.getTimeInMillis();
ZonedDateTime now = ZonedDateTime.now(this.midnight.getZone());
now.minus(_retainDays, ChronoUnit.DAYS);
long expired = now.toInstant().toEpochMilli();

File file= new File(_filename);
File dir = new File(file.getParent());
Expand Down

0 comments on commit 9f317de

Please sign in to comment.