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

min-safe-ts: fix MinSafeTS might be set to MaxUint64 permanently #994

Merged
merged 5 commits into from
Oct 9, 2023

Conversation

HuSharp
Copy link
Member

@HuSharp HuSharp commented Sep 27, 2023

close #991

The core question is the getter

  • we introduce PD API to not execute go func which for KV request, resulting in not updating safeTSMap.
    • updateMinSafeTS relies on safeTSMap which makes sense(because actually, we can call updateMinSafeTS to kvReuqestUpdater[to indicate func base]).
  • And we need to update minsafeTS to make sure when API fails we can fall back to the original way which is by kv request.
  • But the core problem is: updateMinSafeTS will return maxUnit64 when the first kv request returns 0 and then although PD API returns correctly[maybe kv is not initialized], TS can not change maxUnit64.
    • to resolve this question, we need to regard maxUnit64 as 0 which means there is an initial state.

@HuSharp HuSharp marked this pull request as ready for review October 7, 2023 03:32
Signed-off-by: husharp <jinhao.hu@pingcap.com>
@HuSharp
Copy link
Member Author

HuSharp commented Oct 7, 2023

@JmPotato PTAL, thx!

tikv/kv.go Outdated
Comment on lines 503 to 505
if val.(uint64) == uint64(math.MaxUint64) {
return 0
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key point is to prevent the MinSafeTS from not being updated permanently because of MaxUint64. We should handle it from the updater rather than just the getter.

Copy link
Member Author

@HuSharp HuSharp Oct 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core question is the getter

  • we introduce PD API to not execute go func which for KV request, resulting in not updating safeTSMap.
    • updateMinSafeTS relies on safeTSMap which makes sense(because actually, we can call updateMinSafeTS to kvReuqestUpdater[to indicate func base]).
  • And we need to update minsafeTS to make sure when API fails we can fall back to the original way which is by kv request.
  • But the core problem is: updateMinSafeTS will return maxUnit64 when the first kv request returns 0[maybe kv is not initialized] and then although PD API returns correctly, TS can not change maxUnit64.
    • to resolve this question, we need to regard maxUnit64 as 0 which means there is an initial state.

Signed-off-by: husharp <jinhao.hu@pingcap.com>
@HuSharp
Copy link
Member Author

HuSharp commented Oct 8, 2023

@nolouch PTAL as well, thx!

Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

tikv/kv.go Outdated Show resolved Hide resolved
Signed-off-by: husharp <jinhao.hu@pingcap.com>
Signed-off-by: husharp <jinhao.hu@pingcap.com>
@bufferflies bufferflies merged commit 652de4d into tikv:master Oct 9, 2023
10 checks passed
@HuSharp HuSharp deleted the fix_cluster_init_zero branch October 9, 2023 02:46
iosmanthus added a commit that referenced this pull request Dec 20, 2023
Co-authored-by: cfzjywxk <cfzjywxk@gmail.com>
Co-authored-by: cfzjywxk <lsswxrxr@163.com>
Co-authored-by: disksing <i@disksing.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: zzm <zhouzemin@pingcap.com>
Co-authored-by: husharp <jinhao.hu@pingcap.com>
Co-authored-by: you06 <you1474600@gmail.com>
Co-authored-by: buffer <doufuxiaowangzi@gmail.com>
Co-authored-by: 3pointer <qdlc2010@gmail.com>
Co-authored-by: buffer <1045931706@qq.com>
Co-authored-by: husharp <ihusharp@gmail.com>
Co-authored-by: crazycs520 <crazycs520@gmail.com>
Co-authored-by: Smilencer <smityz@qq.com>
Co-authored-by: ShuNing <nolouch@gmail.com>
Co-authored-by: zyguan <zhongyangguan@gmail.com>
Co-authored-by: Jack Yu <jackysp@gmail.com>
Co-authored-by: Weizhen Wang <wangweizhen@pingcap.com>
Co-authored-by: lucasliang <nkcs_lykx@hotmail.com>
Co-authored-by: healthwaite <148101100+healthwaite@users.noreply.github.com>
Co-authored-by: xufei <xufeixw@mail.ustc.edu.cn>
Co-authored-by: JmPotato <ghzpotato@gmail.com>
Co-authored-by: ekexium <eke@fastmail.com>
Co-authored-by: 山岚 <36239017+YuJuncen@users.noreply.github.com>
Co-authored-by: glorv <glorvs@163.com>
Co-authored-by: Yongbo Jiang <cabinfeveroier@gmail.com>
resolve locks interface for tidb gc_worker (#945)
fix some issues of replica selector (#910)  (#942)
fix some issues of replica selector (#910)
fix issue of configure kv timeout not work when disable batch client (#980)
fix batch-client wait too long and add some metrics (#973)
fix batch-client wait too long and add some metrics (#973)" (#984)
fix data race at the aggressiveLockingDirty (#913)
fix MinSafeTS might be set to MaxUint64 permanently (#994)
fix: fix invalid nil pointer when trying to record Store.SlownessStat. (#1017)
Fix batch client batchSendLoop panic (#1021)
fix request source tag unset (#1025)
Fix comment of `SuspendTime` (#1057)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MinSafeTS might be set to MaxUint64 permanently
5 participants