update log to pingcap/log #217
Conversation
|
@kennytm PTAL again |
Co-Authored-By: WangXiangUSTC <xiang13225080@163.com>
Co-Authored-By: WangXiangUSTC <xiang13225080@163.com>
Co-Authored-By: WangXiangUSTC <xiang13225080@163.com>
This reverts commit 839a37c.
pkg/diff/diff.go
Outdated
| @@ -583,7 +584,7 @@ func getChunkRows(ctx context.Context, db *sql.DB, schema, table string, tableIn | |||
| query := fmt.Sprintf("SELECT /*!40001 SQL_NO_CACHE */ %s FROM `%s`.`%s` WHERE %s ORDER BY %s%s", | |||
| columns, schema, table, where, strings.Join(orderKeys, ","), collation) | |||
|
|
|||
| log.Debugf("select data by sql %s, args: %v", query, args) | |||
| log.Debug("select data", zap.String("sql", query), zap.Any("args", args)) | |||
There was a problem hiding this comment.
replace zap.Any with zap.Reflect?
same in tidb-binlog/driver/example/mysql/mysql.go L91.
There was a problem hiding this comment.
https://godoc.org/go.uber.org/zap#Reflect
in the document, have a word Outside tests, Any is always a better choice, should we replace Any to Reflect? @kennytm
There was a problem hiding this comment.
I think this doc can be exchanged to something like
Reflect is always a better choice
😬
There was a problem hiding this comment.
but it's relatively slow and allocation-heavy, seems not suggest
😬
There was a problem hiding this comment.
- If the type is
interface{},Anyis indeed better thanReflectsince it could fallback to faster methods if it turns out to be a string/int/etc. Howeverargsis an[]interface{}. - When you know none of the predefined methods applies,
Reflectis a better choice thanAny, sinceAnywill eventually give up and callReflectanyway. - Every other method is a better choice than both
AnyandReflect. - You could implement ArrayMarshaler to use
zap.Array, for instance.
There was a problem hiding this comment.
yeh. the problem here is args []interface{}.
There was a problem hiding this comment.
I do a bench mark test for []interface{}, reflect is a little better than any
BenchmarkAnyLog-4 500000 15240 ns/op
BenchmarkReflectLog-4 500000 15520 ns/op
Already changed, PTAL again
What problem does this PR solve?
update log to pingcap/log
What is changed and how it works?
update log pkg
Check List
Tests
Related changes