domain: use with kv timeout feature schema load kv timeout #48017
Conversation
ti-chi-bot
bot
added
release-note-none
|
Hi @cfzjywxk. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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/test-infra repository. |
| @@ -184,6 +184,7 @@ func (do *Domain) loadInfoSchema(startTS uint64) (infoschema.InfoSchema, bool, i | |||
| loadSchemaDurationTotal.Observe(time.Since(beginTime).Seconds()) | |||
| }() | |||
| snapshot := do.store.GetSnapshot(kv.NewVersion(startTS)) | |||
| snapshot.SetOption(kv.TiKVClientReadTimeout, uint64(3000)) // 3000ms. | |||
There was a problem hiding this comment.
What is the default value if not set?
There was a problem hiding this comment.
Should we have snapshot.SetOption(kv.ReplicaRead, kv.ReplicaReadMixed) set as well, otherwise it won't retry on replica, right?
There was a problem hiding this comment.
@zimulala
The default value of Get is 30s and BatchGet is 60s.
There was a problem hiding this comment.
@Tema
If snapshot.SetOption(kv.ReplicaRead, kv.ReplicaReadMixed) is not set, the leader peer would be tried first. If timeout happens the error is catched, the replica selector would be transferfred to TryFollower automatically , then follower read would be used on the other peers.
So it's not necessary to involve follower read in the first try I think.
There was a problem hiding this comment.
The default value of Get is 30s and BatchGet is 60s.
If that is true, then retry does not go to replica. I've tested this branch without changes in this PR by setting lease=300s and it didn't help. If the default timeout is 30s then lease would be able to renew by falling back to heathy replica, which I didn't observe in the experiment. So the default timeout is much bigger, does not exists at all or retry does not go to replica for some reason.
There was a problem hiding this comment.
Thanks @cfzjywxk! I've tested this patch on our test bed and it works pretty well!
|
@Tema: adding LGTM is restricted to approvers and reviewers in OWNERS files. In 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/test-infra repository. |
ti-chi-bot
bot
added
needs-1-more-lgtm
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: crazycs520, Tema, you06 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
cfzjywxk
merged commit da06a17
into
pingcap:tidb-6.5-with-kv-timeout-feature
What problem does this PR solve?
Issue Number: ref #48124
Problem Summary:
Use kv timeout feature for schema reload.
What is changed and how it works?
When the leader peer of meta region is slow, use kv timeout could help alleviated by enabling the KV read timeout, avoid errors like "schema lease expire". For example when injecting slowness into the meta region TiKV node,
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.