-
Notifications
You must be signed in to change notification settings - Fork 267
feat: Support comet native log level conf #2379
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2379 +/- ##
============================================
+ Coverage 56.12% 57.44% +1.31%
- Complexity 976 1297 +321
============================================
Files 119 147 +28
Lines 11743 13424 +1681
Branches 2251 2351 +100
============================================
+ Hits 6591 7711 +1120
- Misses 4012 4451 +439
- Partials 1140 1262 +122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@wForget Could you a note to the debugging guide? |
Sure. Also, can I change these log properties to environments? It seems easier to configure them in spark using something like like |
Previously, the |
|
@andygrove Could you please take another look? I added the document. |
| LOG.info( | ||
| "Couldn't locate log file from either COMET_CONF_DIR or comet.log.file.path. " | ||
| + "Using default log configuration which emits to stdout"); | ||
| + "Using default log configuration with {} log level which emits to stderr", |
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.
Corrected to emits to stderr
datafusion-comet/native/core/src/lib.rs
Line 106 in a53e090
| .target(Target::Stderr) |
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.
Why change to stderr? Shouldn't the default log level should be info, emitting to stdout?
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.
Why change to stderr? Shouldn't the default log level should be info, emitting to stdout?
This is just a log fix, the behavior change was introduced by #164
comphead
left a comment
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.
LGTM thanks @wForget
2c4c805 to
597b362
Compare
andygrove
left a comment
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.
Thanks @wForget
Which issue does this PR close?
Closes #2309
Rationale for this change
Makes it easier for us to modify the native log level
What changes are included in this PR?
Add
COMET_LOG_LEVELenv to allow specifying native log levelHow are these changes tested?
Adding

COMET_LOG_LEVEL=DEBUGoverrides default log level: