Allow logger override #699
Conversation
|
@lance6716 tagging you, let me know if there is somebody better to tag |
| @@ -107,7 +105,6 @@ func (c *Canal) runSyncBinlog() error { | |||
| if e != ErrExcludedTable && | |||
| e != schema.ErrTableNotExist && | |||
| e != schema.ErrMissingTableMeta { | |||
| log.Errorf("handle rows event at (%s, %d) error %v", pos.Name, curPos, err) | |||
There was a problem hiding this comment.
why not replace this with a call to c.cfg.Logger.Errorf?
There was a problem hiding this comment.
I can if you want!
There was a problem hiding this comment.
I don't think we should log errors in the library, just return them
There was a problem hiding this comment.
that seems like the consistent thing to do
There was a problem hiding this comment.
sure I will change
cameron-p-m
force-pushed
the
fix/allow-setting-logger
branch
from
59d2309
to
7b2f3ea
Compare
|
thanks for your pr! I'm quite busy today, I will take a look tomorrow. |
|
@lance6716 Sounds good. If there is anything I need to do to bump the version, let me know. I would like to start using this without using a fork :) |
cameron-p-m
force-pushed
the
fix/allow-setting-logger
branch
from
2aecc20
to
a4d75d6
Compare
|
@lance6716 I made the same change in binlog syncer too now |
There was a problem hiding this comment.
found that there's still some reference to global logger, like go-mysqldump, https://github.com/cameron-p-m/go-mysql/blob/fix/allow-setting-logger/mysql/mariadb_gtid.go#L96 . But it's OK to left them for a future PR.
unit tests failed, rest lgtm
@skoef do you have any comment?
|
I'm fine with this! Perhaps sometimes we can refactor using |
|
@lance6716 updated and fixed tests |
|
@lance6716 when we can use this feature, v1.6? is there any way to use latest master code |
|
@atercattus Do you have a release plan? @naughtyGitCat Currently you can specify the master branch or commit hash in go.mod https://go.dev/ref/mod#vcs-branch |
|
@lance6716, I'll do it soon. |
Problem
Ideally libraries don't have a logger at all. But this is a smaller change by allowing a user to set the Logger. This will allow the previous behaviour if needed.
Closes: #698 and maybe others
Feedback?
Should errors ever log or just return?