pump.Storage: fix possible panic if start ts is greater than max commit ts when pull binlog #758
Conversation
Co-Authored-By: Ian <ArGregoryIan@gmail.com>
pump/storage/storage.go
Outdated
| limitTS := atomic.LoadInt64(&a.maxCommitTS) + 1 | ||
| if startTS > limitTS { | ||
| log.Warn("last ts is greater than pump's max commit ts", zap.Int64("last ts", startTS-1), zap.Int64("max commit ts", limitTS-1)) | ||
| time.Sleep(time.Second * 10) |
There was a problem hiding this comment.
10 second is too long, how about return directly and let client to handle when to pull again
There was a problem hiding this comment.
if so, you also need to check logic of drainer
There was a problem hiding this comment.
can't return here, otherwise, drainer will not get binlog forever and need restart
There was a problem hiding this comment.
and this need update a lot of code, I think just wait is ok
There was a problem hiding this comment.
10 seconds is too long
There was a problem hiding this comment.
update to 1 second
|
Rest LGTM |
…tidb-binlog into xiang/fix_leveldb_panic
| @@ -1114,9 +1115,15 @@ func (a *Append) PullCommitBinlog(ctx context.Context, last int64) <-chan []byte | |||
|
|
|||
| for { | |||
| startTS := last + 1 | |||
| limitTS := atomic.LoadInt64(&a.maxCommitTS) + 1 | |||
| if startTS > limitTS { | |||
There was a problem hiding this comment.
just set limitTS to be startTS + 1 if startTS >= limitTS to by pass the bug
don't log warn log and sleep 10 second
and add some commend about the bug
There was a problem hiding this comment.
will it cause some bug if query some binlog is greater than the max commit ts?
There was a problem hiding this comment.
add a comment
There was a problem hiding this comment.
correct:
set limitTS to be startTS if startTS > limitTS if the bug about goleveldb will only trigger when
start > limit
[startTS, limitTS = startTS) will get no any binlog
|
LGTM |
| if startTS > limitTS { | ||
| // if range's start is greater than limit, may cause panic, see https://github.com/syndtr/goleveldb/issues/224 for detail. | ||
| pLog.Print(labelWrongRange, func() { | ||
| log.Warn("last ts is greater than pump's max commit ts", zap.Int64("last ts", startTS-1), zap.Int64("max commit ts", limitTS-1)) |
There was a problem hiding this comment.
This is actually a normal case, so don't add warn log.
like the MaxCommitTS = 10 in pump, checkpointTS = 10 for drainer,
so check for if we having newer binlogs by [checkpointTS + 1, MaxCommitTS + 1)
There was a problem hiding this comment.
in normal case startTS is equal to limitTS, but will not greater than it?
There was a problem hiding this comment.
look yes, but why will trigger this?
|
/run-all-tests tidb=release-3.0 tikv=release-3.0 pd=release-3.0 |
WangXiangUSTC
added
already-cherry-pick-2.1
and removed
needs-cherry-pick-release-2.1
labels
What problem does this PR solve?
in pull binlog, if start ts is greater than max commit ts, pump may panic because goleveldb's bug syndtr/goleveldb#224
What is changed and how it works?
add a check in storage
and update some log by the way
Check List
Tests
Related changes