Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An invalid err != nil check may cause the merged TS inconsistent #8524

Closed
JmPotato opened this issue Aug 14, 2024 · 0 comments · Fixed by #8523
Closed

An invalid err != nil check may cause the merged TS inconsistent #8524

JmPotato opened this issue Aug 14, 2024 · 0 comments · Fixed by #8523
Labels
affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. affects-8.3 component/keyspace Key space. component/tso Timestamp Oracle. severity/critical type/bug The issue is confirmed as a bug.

Comments

@JmPotato
Copy link
Member

Bug Report

var mergedTS time.Time
for _, id := range mergeList {
ts, err := kgm.tsoSvcStorage.LoadTimestamp(endpoint.KeyspaceGroupGlobalTSPath(id))
if err != nil {
log.Error("failed to load the keyspace group TSO",
zap.String("member", kgm.tsoServiceID.ServiceAddr),
zap.Uint32("merge-target-id", mergeTargetID),
zap.Any("merge-list", mergeList),
zap.Uint32("merge-id", id),
zap.Time("ts", ts),
zap.Error(err))
break
}
if ts.After(mergedTS) {
mergedTS = ts
}
}
if err != nil {
continue
}

Since the err in the loop is a local variable, so the check at line 1382 will always be false.

ti-chi-bot bot pushed a commit that referenced this issue Aug 14, 2024
close #8524

Use the continue label to retry the whole merge loop correctly.

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Co-authored-by: JmPotato <ghzpotato@gmail.com>
JmPotato added a commit to JmPotato/pd that referenced this issue Aug 14, 2024
close tikv#8524

Use the continue label to retry the whole merge loop correctly.

Signed-off-by: JmPotato <ghzpotato@gmail.com>
ti-chi-bot bot pushed a commit that referenced this issue Aug 14, 2024
close #8524

Use the continue label to retry the whole merge loop correctly.

Signed-off-by: JmPotato <ghzpotato@gmail.com>

Co-authored-by: JmPotato <ghzpotato@gmail.com>
ti-chi-bot bot added a commit that referenced this issue Sep 14, 2024
close #8524

Use the continue label to retry the whole merge loop correctly.

Signed-off-by: JmPotato <ghzpotato@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>

Co-authored-by: JmPotato <ghzpotato@gmail.com>
Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
Co-authored-by: Ryan Leung <rleungx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. affects-8.3 component/keyspace Key space. component/tso Timestamp Oracle. severity/critical type/bug The issue is confirmed as a bug.
Projects
Development

Successfully merging a pull request may close this issue.

3 participants