checksum: use gc ttl api for checksum gc safepoint in v4.0 cluster #396
Conversation
|
IIRC @overvenus suggested that the GC-TTL API cannot be used to affect the TiDB GC lifetime. Could you confirm with the TiDB team whether this change is effective? Thanks. |
|
Confirmed with @MyonKeminta, service GC safe point (GC TTL) should resolve this problem. |
lightning/restore/restore.go
Outdated
|
|
||
| // for v4.0.0 or upper, we can use the gc ttl api | ||
| var manager ChecksumManager | ||
| if pdVersion.Major >= 4 { |
There was a problem hiding this comment.
I remember it's not all 4.0 version support gc-ttl...
There was a problem hiding this comment.
since v4.0.0.rc.1 pd has supported this interface, so I think this check is ok?
lightning/restore/restore.go
Outdated
|
|
||
| updateTick := time.NewTicker(updateGapTime) | ||
|
|
||
| _ = m.updateGCTTL(ctx) |
There was a problem hiding this comment.
How about add a log for potential error?
lightning/restore/restore.go
Outdated
| return | ||
| case <-updateTick.C: | ||
| if err := m.updateGCTTL(ctx); err != nil { | ||
| log.L().Warn("failed to update service safe point, backup may fail if gc triggered", |
There was a problem hiding this comment.
LGTM, but why not use one service ts point for checksum. not per table.
lightning only set service gc safepoint we run table checksum, currently approach only set gc safepoint when start checksum. So the safepoint should be always the newest. if lightning will run many hours, the safe point can be updated on time
|
/run-all-tests |
lightning/restore/restore.go
Outdated
| pdAddr := rc.cfg.TiDB.PdAddr | ||
| pdVersion, err := common.FetchPDVersion(rc.tls, pdAddr) | ||
| if err != nil { | ||
| return nil, errors.Trace(err) | ||
| } |
There was a problem hiding this comment.
fetch PD version will fail when using "TiDB backend" outside of the cluster (i.e. the pd-addr is inaccessible).
you should return a no-op manager for TiDB backend instead of an error.
There was a problem hiding this comment.
fixed, if checksum is not enabled, return nil instead
lightning/restore/restore.go
Outdated
| manager, ok := ctx.Value(&gcLifeTimeKey).(*gcLifeTimeManager) | ||
| if !ok { | ||
| return nil, errors.New("No gcLifeTimeManager found in context, check context initialization") | ||
| type ChecksumManager interface { |
There was a problem hiding this comment.
perhaps move these checksum managers into another file. this file is too large 😂
3pointer
added
status/LGT2
What problem does this PR solve?
close #389
What is changed and how it works?
Check List
Tests
Side effects
Related changes