-
Notifications
You must be signed in to change notification settings - Fork 289
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
log(ticdc): output the zap internal errors to stderr #3778
log(ticdc): output the zap internal errors to stderr #3778
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## master #3778 +/- ##
================================================
- Coverage 57.0741% 55.2096% -1.8646%
================================================
Files 478 486 +8
Lines 56551 60033 +3482
================================================
+ Hits 32276 33144 +868
- Misses 20978 23527 +2549
- Partials 3297 3362 +65 |
Does it output all error-level log to "stderr"? If so, I think it's a bit verbose. |
@overvenus No, all messages logged by cdc is written to log file as before. This PR only affects the output of internal errors of zap. For example, the following message will be sent to stderr:
Reference: https://github.com/uber-go/zap/blob/master/zapcore/entry.go#L222-L225 |
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.
Do we have a way to test it, please?
I will take a look at how zap lib test for this scenario. |
Co-authored-by: Neil Shen <overvenus@gmail.com>
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 9b45545
|
/merge |
@zhaoxinyu: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
/assign @hi-rustin |
/run-dm-integration-test |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 78b170a
|
What problem does this PR solve?
#3362
What is changed and how it works?
Use a new field
ErrorOutputPath
in log.Config to output the internal errors of zap (eg. no space left on device). See pingcap/log#24 for more detail.Previously, the log lib used in cdc outputs the log internal errors to the same log file designated in
log.FileLogConfig.Filename
. So if the underlying disk is full, those errors are silently discarded, which makes users hard to do troubleshooting.Check List
Tests
Release note