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 default max file size in FileConfig.java #24060

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

VanillaSpoon
Copy link
Contributor

@VanillaSpoon VanillaSpoon commented Mar 2, 2022

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


-@ConfigItem(defaultValueDocumentation = "10")
+@ConfigItem(defaultValue = "10M")

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

Thanks for the quick PR @VanillaSpoon 🥄

@VanillaSpoon
Copy link
Contributor Author

Thanks @machi1990 😊

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 2, 2022

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 60c50ae

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
JVM Tests - JDK 17 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: core/test-extension/deployment 
! Skipped: integration-tests/test-extension 

📦 core/test-extension/deployment

io.quarkus.logging.CategoryConfiguredHandlerTest.consoleOutputTest line 41 - More details - Source on GitHub

java.lang.AssertionError: 
[Extracted: class] 
Expecting actual:

⚙️ JVM Tests - JDK 11 Windows #

- Failing: core/test-extension/deployment 
! Skipped: integration-tests/test-extension 

📦 core/test-extension/deployment

io.quarkus.logging.CategoryConfiguredHandlerTest.consoleOutputTest line 41 - More details - Source on GitHub

java.lang.AssertionError: 
[Extracted: class] 
Expecting actual:

⚙️ JVM Tests - JDK 17 #

- Failing: core/test-extension/deployment 
! Skipped: integration-tests/test-extension 

📦 core/test-extension/deployment

io.quarkus.logging.CategoryConfiguredHandlerTest.consoleOutputTest line 41 - More details - Source on GitHub

java.lang.AssertionError: 
[Extracted: class] 
Expecting actual:

@machi1990
Copy link
Member

Thanks @machi1990 blush

There are some tests failing. Can you have a look?

@VanillaSpoon
Copy link
Contributor Author

Of course 😊 I'll get back to you @machi1990

@machi1990
Copy link
Member

Of course blush I'll get back to you @machi1990

Thanks, let me know if you'll need some inputs.

@machi1990 machi1990 added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 2, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 4, 2022

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 89559ea

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
JVM Tests - JDK 17 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: core/test-extension/deployment 
! Skipped: integration-tests/test-extension 

📦 core/test-extension/deployment

io.quarkus.logging.CategoryConfiguredHandlerTest.consoleOutputTest line 41 - More details - Source on GitHub

java.lang.AssertionError: 
[Extracted: class] 
Expecting actual:

⚙️ JVM Tests - JDK 11 Windows #

- Failing: core/test-extension/deployment 
! Skipped: integration-tests/test-extension 

📦 core/test-extension/deployment

io.quarkus.logging.CategoryConfiguredHandlerTest.consoleOutputTest line 41 - More details - Source on GitHub

java.lang.AssertionError: 
[Extracted: class] 
Expecting actual:

⚙️ JVM Tests - JDK 17 #

- Failing: core/test-extension/deployment 
! Skipped: integration-tests/test-extension 

📦 core/test-extension/deployment

io.quarkus.logging.CategoryConfiguredHandlerTest.consoleOutputTest line 41 - More details - Source on GitHub

java.lang.AssertionError: 
[Extracted: class] 
Expecting actual:

@gsmet
Copy link
Member

gsmet commented Mar 9, 2022

Hmmm, so I worked on fixing the tests and AFAICS, log rotation was disabled until now or at least it was not using a size rotating appender.

I think enabling rotation by default makes sense.

@jamezp do you see any red flags with this change?

@jamezp
Copy link
Contributor

jamezp commented Mar 9, 2022

@gsmet Seems fine to me. The only question I'd have is if you'd prefer to default to a SizeRotatingFileHandler or a PeriodicRotatingFileHandler. The size one has better cleanup capabilities. However, it should be noted with the RotationConfig.fileSuffix of a date the files will NOT be deleted. You'll end up with no files being deleted on the next rotation.

@VanillaSpoon
Copy link
Contributor Author

Sorry for not getting this one sorted guys. Thanks for the change @gsmet

@machi1990
Copy link
Member

@gsmet Seems fine to me. The only question I'd have is if you'd prefer to default to a SizeRotatingFileHandler or a PeriodicRotatingFileHandler. The size one has better cleanup capabilities. However, it should be noted with the RotationConfig.fileSuffix of a date the files will NOT be deleted. You'll end up with no files being deleted on the next rotation.

+1 on defaulting to using SizeRotatingFileHandler.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/logging triage/waiting-for-ci Ready to merge when CI successfully finishes
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
4 participants