Add Executor Service To Delete Logs When log_daily Is Enabled #1754
Conversation
TimeBasedTriggeringPolicy
buildDailyRollingAppender
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1754 +/- ##
============================================
+ Coverage 70.81% 70.85% +0.04%
+ Complexity 9954 9946 -8
============================================
Files 827 826 -1
Lines 39917 39868 -49
Branches 6059 6040 -19
============================================
- Hits 28267 28250 -17
+ Misses 8924 8891 -33
- Partials 2726 2727 +1 ☔ View full report in Codecov by Sentry. |
newrelic-agent/src/main/java/com/newrelic/agent/logging/ClearExpiredLogFilesRunnable.java
Outdated
Show resolved
Hide resolved
newrelic-agent/src/main/java/com/newrelic/agent/logging/ClearExpiredLogFilesRunnable.java
Outdated
Show resolved
Hide resolved
|
|
||
| // Define the pattern for matching log file names | ||
| pattern = Pattern.compile(fileNamePrefix.replace(".", "\\.") | ||
| + "\\.(\\d{4}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01]))(\\.\\d)?$"); |
There was a problem hiding this comment.
This could be a constant. It would make the code easier to read, and the name of the constant will document what this part of the regex is.
There was a problem hiding this comment.
created constant DATE_REGEX as a class field
|
|
||
| @Override | ||
| public void run() { | ||
| Thread.currentThread().setName("New Relic Expiring Log File Cleanup"); |
There was a problem hiding this comment.
This could be done in the executor.
There was a problem hiding this comment.
moved to the executor 👍
|
|
||
| String filePattern = fileName + ".%d{yyyy-MM-dd}"; | ||
| if (logLimitBytes > 0) { | ||
| // If we might roll within a day, use a number ordering suffix | ||
| filePattern = fileName + ".%d{yyyy-MM-dd}.%i"; | ||
| } | ||
|
|
||
| Path directory = new File(this.path).toPath(); | ||
| ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor(); |
There was a problem hiding this comment.
I believe this will generate non daemon threads. Which will prevent the application from shutting down.
There was a problem hiding this comment.
used a ThreadFactory to explicitly configure the generated threads used for the executorService as daemon threads
| public void tearDown() throws IOException { | ||
| if (tempDir != null && Files.exists(tempDir)) { | ||
| Files.walk(tempDir) | ||
| .sorted((a, b) -> -a.compareTo(b)) |
There was a problem hiding this comment.
The only test that requires files to be sorted prior to deletion is testDirectoryWithMixedFiles. This test contains a subdirectory with files so the sorting ensures that all files are deleted before subdirectories are deleted. Without sorting, this test throws a DirectoryNotEmptyException when attempting to delete the parent directory and the test fails.
|
|
||
| @Test | ||
| public void testRunnableAgainstEmptyLogDirectory() throws IOException { | ||
| tempDir = Files.createTempDirectory("emptyLogDir"); |
There was a problem hiding this comment.
do you need to create another temp directory? The code in @before just created one.
| date = calendar.getTime(); | ||
| } | ||
| } | ||
| } |
switched usage of Date with LocalDate
Fixes #1688