Repository navigation
Conversation
|
/cc @OliverS929 @D3Hunter |
There was a problem hiding this comment.
Code Review
This pull request updates the project to Go 1.25.9 and performs a significant upgrade of various dependencies, most notably go-mysql and tidb. The core changes involve refactoring GTID handling to align with the updated go-mysql library, including the transition from UUIDSet to MysqlGTIDSet and structural changes to MariaDB GTID sets. Feedback from the review highlights a potential side-effect in GTID incrementing that should be addressed by working on a copy of the data. Additionally, there are recommendations for improving code maintainability through the use of helper functions for repeated logic, removing redundant assignments, and simplifying loops where variables are unused.
| mariaGTID := singleGTID.(*gmysql.MariadbGTID) | ||
| mariaGTID.SequenceNumber++ | ||
| gtidSet := new(gmysql.MariadbGTIDSet) | ||
| gtidSet.Sets = map[uint32]map[uint32]*gmysql.MariadbGTID{ | ||
| mariaGTID.DomainID: { | ||
| mariaGTID.ServerID: mariaGTID, | ||
| }, | ||
| gtidSet.Sets = map[uint32]*gmysql.MariadbGTID{ | ||
| mariaGTID.DomainID: mariaGTID, | ||
| } | ||
| clone = gtidSet |
There was a problem hiding this comment.
In the MariaDB flavor case, mariaGTID is a pointer to an element within the original gSet. Modifying mariaGTID.SequenceNumber directly causes a side effect on the input gSet. To ensure GTIDIncrease is pure and doesn't modify its input, you should work on a copy of the GTID struct.
| mariaGTID := singleGTID.(*gmysql.MariadbGTID) | |
| mariaGTID.SequenceNumber++ | |
| gtidSet := new(gmysql.MariadbGTIDSet) | |
| gtidSet.Sets = map[uint32]map[uint32]*gmysql.MariadbGTID{ | |
| mariaGTID.DomainID: { | |
| mariaGTID.ServerID: mariaGTID, | |
| }, | |
| gtidSet.Sets = map[uint32]*gmysql.MariadbGTID{ | |
| mariaGTID.DomainID: mariaGTID, | |
| } | |
| clone = gtidSet | |
| mariaGTID := *singleGTID.(*gmysql.MariadbGTID) | |
| mariaGTID.SequenceNumber++ | |
| gtidSet := new(gmysql.MariadbGTIDSet) | |
| gtidSet.Sets = map[uint32]*gmysql.MariadbGTID{ | |
| mariaGTID.DomainID: &mariaGTID, | |
| } | |
| clone = gtidSet |
| for u, tags := range *mSet { | ||
| sid = u | ||
| for _, intervals := range tags { | ||
| interval = intervals[0] | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for extracting the single SID and Interval from a MysqlGTIDSet is repeated here and in newGenerator (in generator.go). Since DM requires these sets to have exactly one entry (as enforced by verifySingleGTID), consider moving this extraction logic into a helper function to improve code reuse and maintainability.
| for tag, intervals := range tags { | ||
| intervals[0].Start++ | ||
| intervals[0].Stop++ | ||
| (*mSet)[u][tag] = intervals |
There was a problem hiding this comment.
| var sid uuid.UUID | ||
| var tags map[gmysql.Tag]gmysql.IntervalSlice | ||
| for sid, tags = range *mysqlGTIDs { | ||
| } | ||
| var uuidSet *gmysql.UUIDSet | ||
| for _, uuidSet = range mysqlGTIDs.Sets { | ||
| _ = sid |
There was a problem hiding this comment.
59316e0 to
a6afe34
Compare
|
/retest |
#4578) ### What Update TiFlow latest CI images to the newer Go 1.25 image tag: - `ghcr.io/pingcap-qe/ci/jenkins:v2026.4.12-10-gc29110c-go1.25` for jenkins job - `ghcr.io/pingcap-qe/ci/base:v2026.4.12-10-gc29110c-go1.25` for the Prow unit test job ### Why TiFlow PR pingcap/tiflow#12628 updates `go.mod` to `go 1.25.9`. Some CI jobs were still running with older Go 1.25 images, causing errors like: ```text compile: version "go1.25.9" does not match go tool version "go1.25.8" ``` ### Test result I have run replay tests for all failed tasks. The Go version mismatch issue is now fully resolved. The remaining failures are either integration test failures inherent to the TiFlow PR itself, or runtime issues on individual Jenkins/K8s agents. These are not directly related to the current upgrade of the Go image. --------- Signed-off-by: lyb <yebin.li@pingcap.com>
|
/test pull-verify |
|
/retest |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest pull-verify |
|
@dveeden: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test pull-verify |
|
/retest |
|
/retest |
Updated go-mysql
go-mysql v1.15.0 (PR pingcap#1080) decodes upstream heartbeat events as a dedicated *replication.HeartbeatEvent type instead of GenericEvent. DM's switches on *replication.GenericEvent no longer match real heartbeats, so the validator never persists checkpoints, the syncer never flushes on idle, and the relay no longer marks heartbeats as ignored. This caused validator_basic to hang with non-zero pending rows. Accept both types since DM still injects its own heartbeats locally as GenericEvent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The test asserted finished_bytes=59674 verbatim, but the tidb-pkg upgrade in this branch ships a slightly different lightning that reports 59754 bytes for the same dumped data. Replace the literal byte count with a backreference assertion that finished_bytes equals total_bytes (and is non-empty), so the test no longer breaks on benign size shifts from lightning. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1c8f644 to
c4f6f36
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. |
|
/test pull-verify |
|
/retest |
|
/retest |
Updated go-mysql
What problem does this PR solve?
Issue Number: closes #12629
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note