Conversation
|
Still outstanding:
|
|
Alright, unit testing is done and I've added GTID support to the following
I DID NOT add GTID support to the following because it's unused code, plus many methods aren't possible to reproduce with GTIDs:
cc @shlomi-noach / @gtowey / @rashiq for review if anyone has time 🙇 |
| // StreamEvents | ||
| func (this *GoMySQLReader) handleRowsEvent(ev *replication.BinlogEvent, rowsEvent *replication.RowsEvent, entriesChannel chan<- *BinlogEntry) error { | ||
| if this.currentCoordinates.SmallerThanOrEquals(&this.LastAppliedRowsEventHint) { | ||
| if !this.migrationContext.UseGTIDs && this.currentCoordinates.SmallerThanOrEquals(&this.LastAppliedRowsEventHint) { |
There was a problem hiding this comment.
Is there a chance that we could also receive a GTID event with an ID that has already been processed? Or is that handled completely by the binlogSyncer.StartSyncGTID method?
| } | ||
| if this.migrationContext.UseGTIDs { | ||
| this.currentCoordinatesMutex.Lock() | ||
| defer this.currentCoordinatesMutex.Unlock() |
There was a problem hiding this comment.
One thing that might be better is to simply move this line to after 132 without using defer. This is because the defer will wait until the method ends to actually release the mutex lock, which is often longer than needed. Since there's literally only one assignment operation between the lock/unlock then there's really no chance for the method to panic, and there's no branches which include a return after the lock.
| defer this.currentCoordinatesMutex.Unlock() | ||
| this.nextCoordinates = this.currentCoordinates | ||
| interval := gomysql.Interval{Start: event.GNO, Stop: event.GNO + 1} | ||
| this.nextCoordinates.GTIDSet.AddSet(gomysql.NewUUIDSet(sid, interval)) |
There was a problem hiding this comment.
this assumes that each GTID event is always one and only one GTID. Which sounds like it's probably correct, I just wanted to ask if that was something you specifically checked on when writing this code.
Description
This PR adds support for MySQL GTIDs for replication binlog positioning. This support will unblock future reliability improvements to the project. Currently
gh-ostsupport the old-style log files and positions only 👎This new support is enabled using the flag
-gtidand it requires thatgtid_mode=ONandenforce_gtid_consitency=ONis setNOTE: after I get some feedback on the approach and have proven it works I will add some unit testing
cc @shlomi-noach
script/cibuildreturns with no formatting errors, build errors or unit test errors.