search

pingcap / tidb-binlog

A tool used to collect and merge tidb's binlog for real-time data backup and synchronization.
Apache License 2.0
292 stars 131 forks source link

switch from juju/errors to pingcap/errors #360

Closed gregwebs closed 5 years ago

gregwebs commented 6 years ago

I tried to do this switch, but what held me back is that tidb is vendored and still using juju/errors, including some of its esoteric APIs. The latest version of tidb has switched to using pingcap/errors. Similarly, pd has switched to pkg/errors. So I am hoping that the vendored tidb can be updated.

One of the main motivations for me to prompt tidb-binlog to do this change is that juju/errors is LGPL code, which makes proper distribution of tidb-binlog more difficult.

iamxy commented 6 years ago

Please update the related vendored codes. @GregoryIan

gregwebs commented 6 years ago

These changes are in 2.1: you may need to wait for the stable 2.1 release

kennytm commented 6 years ago

tidb-binlog has two LGPL dependencies: juju/errors and ngaut/log. Replacing juju/errors is straightforward with TiDB ≥ v2.1.0-rc.3, but ngaut/log is more difficult.

In TiDB the replacement is sirupsen/logrus for logging, and gopkg.in/natefinch/lumberjack.v2 for log rotation. However, binlog supports rotating by hours, whereas the minimum granularity for lumberjack is days. This makes lumberjack not a suitable replacement of ngaut/log.

gregwebs commented 6 years ago

Thanks for looking into this. I am trying out the zap logger now also :).

I much prefer to leave log rotation to an external tool. In general I prefer to log to stdout and leave it up to the deployment details what happens next. Assuming logging to the machine where it is running is not cloud friendly. We would want the option to ship the log elsewhere. Still its just as possible for the deployment to redirect stdout to a file and use a log rotation tool.

gregwebs commented 6 years ago

Would we need to coordinate with the ansible based deployment to see how we can have that deployment ensure log rotation? @LinuxGit

gregwebs commented 6 years ago

Although in the long-run I do think it is best to avoid coupling log rotation to the binlog process, I did see that zap mentions integration with lumberjack.

kennytm commented 6 years ago

Applied lumberjack's recommendation on how to do time-based rotation, so ngaut/log is no longer the blocking issue.

https://github.com/pingcap/tidb-binlog/blob/8f996ce581270fcbe554919a9a68a9195c43e847/pkg/util/util.go#L90-L103

The next issue is that drainer depends on zanmato1984/clickhouse (MIT), which itself depends on juju/errors

https://github.com/zanmato1984/clickhouse/blob/6c32f7b4cd79451e23e7369c500d082815380b3e/lib/column/decimal.go#L9

The only feature used is errors.Trace so it can be entirely replaced by pingcap/errors in go.mod, but I'm not sure if this is "legal".

replace github.com/juju/errors => github.com/pingcap/errors v0.10.1
gregwebs commented 6 years ago

Thanks for figuring out the log rotation!

Replacing a dependency is perfectly legal. We use Gopkg to replace pkg/errors, but you could do the same to replace juju/errors: https://github.com/pingcap/tidb/blob/master/Gopkg.toml#L101. I don't know how to do that technique with go modules yet.

We could also send a pull request to the clickhouse driver. pingcap/errors offers stack traces, which could be very helpful.

july2993 commented 5 years ago

Thanks gregwebs have switch juju/errors to pingcap/error in https://github.com/pingcap/tidb-binlog/pull/464 drop juju/errors in zanmato1984/clickhouse by https://github.com/zanmato1984/clickhouse/pull/1 for ngaut/log, will switch to pingcap/log later. so close this first.