-
Notifications
You must be signed in to change notification settings - Fork 312
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
feat: set max log files to 720 by default, info log only #4787
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
… and that of slow_queries to opt.max_log_files
@Kev1n8 Looks like some CI checks failed, please run those checks locally to ensure CI can pass: # Check code style
cargo clippy --workspace --all-targets -- -D warnings
# Check if doc is up-to-date
make config-docs
# Run all unit tests
cargo test --workspace --all-targets The failure in unit test relates to your changes in configuration file. |
Seems like there's still something wrong. I'll look into it. Any suggestions will be hugely appreciated too! |
@Kev1n8 Looks like there're some typos detected (though those files are not touched in this PR). Cloud you please also fix them? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4787 +/- ##
==========================================
- Coverage 84.45% 84.20% -0.25%
==========================================
Files 1120 1120
Lines 203478 204139 +661
==========================================
+ Hits 171841 171898 +57
- Misses 31637 32241 +604 |
@Kev1n8 Looks like there are some typos in checking https://github.com/GreptimeTeam/greptimedb/actions/runs/11120980442/job/30935597697?pr=4787 It's not related to this PR, but would you like to fix them in this PR too? Thanks a lot. |
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.
Almost LGTM
Sure. |
… when panicing, put `max_l_f` in right position
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.
@Kev1n8 Looks like config api test still fails.
Co-authored-by: Lei, HUANG <[email protected]>
My bad. Thanks for fixing that! |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
Closes #4777.
What's changed and what's your intention?
Exposed
max_log_files
inLoggingOptions
, set it to 720 by default, and buildinfo_log
with this restriction.Checklist
Not sure how to test this. It passed CI tho.