Skip to content
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

Change RotationConfig defualtmax file size in FileConfig.java #24267

Closed
wants to merge 1 commit into from

Conversation

VanillaSpoon
Copy link
Contributor

@VanillaSpoon VanillaSpoon commented Mar 11, 2022

This is a continuation of #24060 as I accidentally removed the fork, which closed the pr.

###Change RotationConfig default max file size in FileConfig.java


In FileConfig.java RotationConfig the default value for Max Size of the log file after which a rotation is executed was set incorrectly.


Fixes #24041

Copy link
Member

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still a bit nervous about this one. I think it warrants a discussion on the list about what should be the default behavior.

Given we had a FileHandler before, I think the rotation was entirely disabled. So it's not just about changing a default value, it's about enabling the rotation by default or not.

And enabling rotation at the Quarkus level could lead to very weird behaviors if an external log rotation is already in place.

@machi1990
Copy link
Member

I'm still a bit nervous about this one. I think it warrants a discussion on the list about what should be the default behavior.

Given we had a FileHandler before, I think the rotation was entirely disabled. So it's not just about changing a default value, it's about enabling the rotation by default or not.

And enabling rotation at the Quarkus level could lead to very weird behaviors if an external log rotation is already in place.

Looking at #24060 (comment) from @jamezp, the SizeRotatingFileHandler which we are now defaulting too has better cleanups. But I do not see what weird behaviours the change could lead if we default to using that. It would good to see what are those.
If we default to enabling rotation, users can still opt out of rotation by setting the property to empty i.e

quarkus.log.handler.file."file-handlers".rotation.max-file-size=

That can be documented in the migration guide.

One other alternative as evoked in #24041 (comment) if we do not want to default to rotating the log file, is to document the default upstream behaviour via the defaultValueDocumentation knob. But the issue with that is we'll need to track upstream updates to make sure if the default changes there, we update the value for defaultValueDocumentation.

Or add a enable-rotation knob which controls if rotation is enabled or disable and not having to rely on the Optional max file size config.

@machi1990
Copy link
Member

Closing as this is superseded by #25072

@machi1990 machi1990 closed this Apr 26, 2022
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documented quarkus.log.handler.file...rotation.max-file-size default value is incorrect
3 participants