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

sql, server: rename server.shutdown.lease_transfer_wait to server.shutdown.lease_transfer_timeout #75520

Closed
ZhouXing19 opened this issue Jan 25, 2022 · 3 comments · Fixed by #109415
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-server-and-security DB Server & Security

Comments

@ZhouXing19
Copy link
Collaborator

ZhouXing19 commented Jan 25, 2022

In the current draining process, at the lease transfer phase, the server keeps retrying forever until all range leases on this draining node have been transferred. The cluster setting server.shutdown.lease_transfer_wait actually only specifies the timeout for each iteration, which means the duration of the whole lease transfer phase is not solely determined by that value. Therefore, it's inaccurate to name this setting as a "wait" period.

Its current name can also be easily mixed with other real waiting period settings (e.g. server.shutdown.drain_wait, server.shutdown.query_wait). Ideally, server.shutdown.lease_transfer_timeout should be documented separately from the other waiting period settings.

Jira issue: CRDB-12707
Epic: CRDB-28893

@ZhouXing19 ZhouXing19 added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jan 25, 2022
@blathers-crl blathers-crl bot added the T-server-and-security DB Server & Security label Jan 27, 2022
@taroface
Copy link
Collaborator

It seems the description also needs to be corrected:

the amount of time a server waits to transfer range leases before proceeding with the rest of the shutdown process

@knz
Copy link
Contributor

knz commented Jan 27, 2022

NB: we do not customarily rename cluster settings, unless there's egregious user confusion, because the process to do so is not easy (we have to maintain cross-version compatibility...)

But documenting their help text and associated docs is super important, yes

@github-actions
Copy link

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-server-and-security DB Server & Security
Projects
Status: Done 21.2
Development

Successfully merging a pull request may close this issue.

4 participants