-
Notifications
You must be signed in to change notification settings - Fork 131
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
kafka.go: Refine log #778
kafka.go: Refine log #778
Conversation
log fatal instead of panic(err), so user can know what really happen instead of like drainer restart by some unknown reason before check 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.
LGTM.
It is still expected that Kafka error should kill Drainer, right?
yes |
/rebuild |
/run-all-tests |
3 similar comments
/run-all-tests |
/run-all-tests |
/run-all-tests |
/build |
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
before update will: ./tools/check/check-tidy.sh go: downloading gopkg.in/stretchr/testify.v1 v1.4.0 github.com/pingcap/tidb-binlog/drainer imports github.com/pingcap/tidb/store/tikv imports github.com/twinj/uuid tested by github.com/twinj/uuid.test imports gopkg.in/stretchr/testify.v1/assert: go.mod has non-....v1 module path "github.com/stretchr/testify" at revision v1.4.0 make: *** [tidy] Error 1
d4065a8
to
acf7269
Compare
/run-all-tests |
/build |
/run-all-tests |
try drop dep of: go: github.com/go-critic/go-critic@v0.0.0-20181204210945-ee9bf5809ead: invalid pseudo-version: does not match version-control timestamp (2019-02-10T22:04:43Z)
1304981
to
98ad968
Compare
@@ -65,8 +65,7 @@ func InitLogger(level string, file string) error { | |||
cfg := &log.Config{ | |||
Level: level, | |||
File: log.FileLogConfig{ | |||
Filename: file, | |||
LogRotate: true, |
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.
no any affect field in older version and is removed now.
/run-all-tests |
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
/run-all-tests |
@july2993 merge failed. |
/merge |
/run-all-tests |
cherry pick to release-2.1 failed |
cherry pick to release-3.0 failed |
cherry pick to release-3.1 failed |
What problem does this PR solve?
refine log.
What is changed and how it works?
log fatal instead of panic(err), so user can know what really happen
instead of like drainer restart by some unknown reason before check
stderr.
Check List
Tests