-
Notifications
You must be signed in to change notification settings - Fork 188
Conversation
/run-all-tests tidb=release-3.0 |
/run-all-tests tidb=release-3.0 |
/run-all-tests tidb=release-3.0 |
/run-all-tests tidb=release-3.0 |
/run-all-tests tidb=release-3.0 |
/run-all-tests tidb=release-3.0 |
/run-all-tests tidb=release-3.0 |
1 similar comment
/run-all-tests tidb=release-3.0 |
@csuzhangxc @lichunzhu PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM
syncer/checkpoint.go
Outdated
if binlog.ComparePosition(pos, minCheckpoint) > 0 { | ||
cp.globalPoint = newBinlogPoint(pos, nil, pos, nil) | ||
if binlog.CompareLocation(location, binlog.NewLocation(cp.cfg.Flavor)) > 0 { | ||
cp.globalPoint = newBinlogPoint(location, location, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create another location
to avoid the reuse of GTIDSet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all addressed in 53bef75
syncer/checkpoint.go
Outdated
@@ -640,7 +659,7 @@ func (cp *RemoteCheckPoint) Load(tctx *tcontext.Context, schemaTracker *schema.T | |||
mSchema = make(map[string]*binlogPoint) | |||
cp.points[cpSchema] = mSchema | |||
} | |||
mSchema[cpTable] = newBinlogPoint(pos, &ti, pos, &ti) | |||
mSchema[cpTable] = newBinlogPoint(location, location, &ti, &ti) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
syncer/checkpoint.go
Outdated
if err != nil { | ||
return err | ||
} | ||
case config.ModeIncrement: | ||
// load meta from task config | ||
if cp.cfg.Meta == nil { | ||
cp.logCtx.L().Warn("don't set meta in increment task-mode") | ||
location1 := binlog.NewLocation(cp.cfg.Flavor) | ||
cp.globalPoint = newBinlogPoint(location1, location1, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
syncer/checkpoint.go
Outdated
if pos != nil { | ||
cp.globalPoint = newBinlogPoint(*pos, nil, *pos, nil) | ||
if location != nil { | ||
cp.globalPoint = newBinlogPoint(*location, *location, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
syncer/checkpoint.go
Outdated
cp.Lock() | ||
defer cp.Unlock() | ||
cp.saveTablePoint(sourceSchema, sourceTable, pos, ti) | ||
cp.saveTablePoint(sourceSchema, sourceTable, point, ti) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do Clone
for the passed in point
? it seems some of the callers do the Clone
, but some don't. If so, ditto for other exported methods like SaveGlobalPoint
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I haven't unified the usage for Clone
, maybe it's better to let the caller to decide whether to allow the function to update the value, if not allowed, can use Clone
.
I add a Clone
here, maybe we can unified the usage later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it's fine.
/run-all-tests tidb=release-3.0 |
/run-all-tests tidb=release-3.0 |
@lichunzhu @csuzhangxc PTAL again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* add new struct Location to save binlog position and gtid set * use Location in syncer unit
* add new struct Location to save binlog position and gtid set * use Location in syncer unit
* add new struct Location to save binlog position and gtid set * use Location in syncer unit
What problem does this PR solve?
syncer unit support GTID
What is changed and how it works?
Location
to save binlog position and gtid setCheck List
Tests
Side effects
Related changes