-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for LOGMGR-139 #282
base: main
Are you sure you want to change the base?
Fix for LOGMGR-139 #282
Conversation
49d462c
to
93bd8a9
Compare
@@ -44,6 +47,13 @@ | |||
private TimeZone timeZone = TimeZone.getDefault(); | |||
private SuffixRotator suffixRotator = SuffixRotator.EMPTY; | |||
|
|||
private enum PruningStrategy { NONE, PERIODS, SIZE }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I think about the other options at a configuration level that are currently available, I am really mostly attracted to leaving the PruningStrategy.PERIODS option, but this is great!
Thank you for the contribution. I'll have to do some thinking on this as we haven't moved forward with this JIRA because of having to make assumptions about which files to remove. What ever we do here would likely need to be ported to the I personally have some reservations about a handler making assumptions on the files it deletes. What I've got in a local branch is somewhat similar to this only it just used the |
Happy to help. Understood on the reservations about assumptions on files eligible for pruning. If we really want to avoid any and all false positives, one option: define an add an additional (required) property - eligibleForPruning - which can take a regex for files eligible to be pruned. Only files that match the regex and pass the other checks get pruned. It's another thing to configure and keep up to date, sure, but it would make the process safer. |
Could you lay out your reservations about the assumption around what files to remove? I read your comment on https://issues.jboss.org/browse/LOGMGR-139. The advice that seems to have been given when this has come up in the past is to configure a cron job similar to this:
(see https://access.redhat.com/solutions/3682431) I'm challenged to see a reason why similar behavior shouldn't be baked into the implementation with a new property (e.g. maxBackupPeriods) and some clear documentation around the delete behavior. Since this new property wouldn't need to have a default limit, it would be backwards compatible and only put the burden of reading about the cleanup behavior on those who choose to adjust their configuration to adopt the new feature. I think to take extra care to match on the suffix with regex would be more challenging, but is also likely completely possible. I'm not sure its important to be that clever, but that is just my opinion. |
Hi @Hillrunner2008 I'm just a bit uncomfortable about assuming file names. If it's documented well I'd probably be okay with it. The other major part is performance. All logging to the handler will be blocked until the rotation is handled. Hopefully this wouldn't cause a huge performance impact, but it could so it needs to be well thought out. I do realize cron is essentially doing the same thing, I'm just more comfortable with it because it's outside the server. It happens at a specific time and doesn't block the server. A little OT, but another thought I've had is a define a separate |
Took a look at LOGMGR-139, and I've got a simple implementation that I think will handle the majority of use cases.
The implementation adds a property, (pruneSize) on the PeriodicRotatingFileHandler. It defaults to zero. If it is set to > 0, it enables log pruning, which automatically removes old log files until some criteria is met. Pruning functionality operates on files in the directory of the current log file only, to a max depth of 1.
The prune size can be set to one of two options:
A long: Prunes files (oldest first) until the total size of pruneable files found <= pruneSize
An integer with a trailing 'P' or 'p': Prunes any log period files older than periods. Ex: "5p".
Files eligible for pruning have the following qualities:
The implementation is simple; it doesn't handle edge cases like changed log directories. It's based on the assumption that the vast majority of users put their logs in one directory that they set once and very rarely change. I've added some tests as well. Hope that it is useful; let me know your thoughts!