tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bugzi...@apache.org
Subject [Bug 61105] Roll log files by default
Date Fri, 09 Jun 2017 20:32:48 GMT
https://bz.apache.org/bugzilla/show_bug.cgi?id=61105

--- Comment #7 from Violeta Georgieva <violetagg@apache.org> ---
Hi,

(In reply to Konstantin Kolinko from comment #5)
> (In reply to Violeta Georgieva from comment #3)
> > Hi,
> > 
> > What do you think about this approach?
> > https://github.com/apache/tomcat/pull/60
> > 
> 
> +    public static final int DEFAULT_MAX_DAYS = 90;
> +    private int maxDays = DEFAULT_MAX_DAYS;
> 
> I do not like the idea of built-in default limit in java code.
> 
> I am open to discuss whether it is feasible for Tomcat 9,
> but such built-in limit cannot be backported to stable versions (8.5 and
> earlier).
> 
> I think it is better to add limits explicitly to the default
> logging.properties configuration.
> 
> 
> +        String sMaxDays = getProperty(className + ".maxDays",
> String.valueOf(DEFAULT_MAX_DAYS));
> +        if (maxDays <= 0) {
> +            try {
> +                maxDays = Integer.parseInt(sMaxDays);
> +            } catch (NumberFormatException ignore) {
> +                // no-op
> +            }
> +        }
> 
> I think the above try/catch block is never executed, as "if (maxDays <= 0)"
> is always false, as maxDays is "90" by default.
> 
> +    private DirectoryStream<Path> streamFilesForDelete() throws IOException
> {
> +        FileTime maxDaysOffset = FileTime.from(Instant.now().minus(maxDays,
> ChronoUnit.DAYS));
> +        return Files.newDirectoryStream(new File(directory).toPath(), path
> -> {
> +            String fileName = path.getFileName().toString();
> +            return fileName.startsWith(prefix) && fileName.endsWith(suffix)
> +                    &&
> Files.getLastModifiedTime(path).compareTo(maxDaysOffset) < 0;
> +        });
> +    }
> 
> I do not like the above.
> 
> 1. "fileName.startsWith(prefix)" will result in false positives.
> 
> 2. I'd prefer to test the date in the file name, not file modification date.
> 
> 
> BTW, for access logs I usually have an empty prefix, grouping the files into
> separate directories by month:
> fileDateFormat="yyyy-MM'/webappname.'yyyy-MM-dd"
> prefix=""
> suffix=".access.log"
> 
> Such feature is not implemented for JULI logging yet. If it were, the
> "fileName.startsWith(prefix)" here would be true for every file.

Thanks for the review. I prepared a new patch where I applied all your
recommendations. Can you take a look at it?

Violeta

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message