https://bz.apache.org/bugzilla/show_bug.cgi?id=61105
--- Comment #7 from Violeta Georgieva <violet...@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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
