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

server, all: rename some cluster settings #109415

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Conversation

knz
Copy link
Contributor

@knz knz commented Aug 24, 2023

Fixes #75520.
Epic: CRDB-28893

The following settings have been renamed, to emphasize how the initial drain wait is a mandatory wait, whereas the others are simply timeouts.

Release note (cli change): The following cluster settings have been renamed; the previous names remain available for
backward-compatibility.

Previous name New Name
server.shutdown.drain_wait server.shutdown.initial_wait
server.shutdown.lease_transfer_wait server.shutdown.lease_transfer_iteration.timeout
server.shutdown.query_wait server.shutdown.queries.timeout
server.shutdown.connection_wait server.shutdown.transactions.timeout
server.shutdown.jobs_wait server.shutdown.jobs.timeout

@knz knz requested review from a team as code owners August 24, 2023 11:56
@knz knz requested review from herkolategan, smg260 and miretskiy and removed request for a team August 24, 2023 11:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss
Copy link
Collaborator

rafiss commented Aug 24, 2023

I think we should name it server.shutdown.transaction.timeout since this logic actually waits for all open transactions (implicit or explicit) to end.

Also, out of curiosity, why server.shutdown.connections.timeout instead of server.shutdown.connection_timeout?

Another naming scheme to consider is: server.shutdown.timeout.connection, server.shutdown.timeout.lease_transfer_iteration, ``server.shutdown.timeout.transaction`, etc. In case that kind of namespace hierarchy is helpful

@rafiss rafiss self-requested a review August 24, 2023 14:52
@knz
Copy link
Contributor Author

knz commented Sep 1, 2023

RFAL

I think we should name it server.shutdown.transaction.timeout since this logic actually waits for all open transactions (implicit or explicit) to end.

Done.

Also, out of curiosity, why server.shutdown.connections.timeout instead of server.shutdown.connection_timeout?
Another naming scheme to consider is: server.shutdown.timeout.connection, server.shutdown.timeout.lease_transfer_iteration, ``server.shutdown.timeout.transaction`, etc. In case that kind of namespace hierarchy is helpful

As per the guidelines:
image

@knz knz force-pushed the 20230824-drain branch 2 times, most recently from 7caa112 to 08d84d0 Compare September 1, 2023 10:10
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

tyvm!

The following settings have been renamed, to emphasize how the initial
drain wait is a *mandatory waits*, whereas the others are
simply *timeouts*.

Release note (cli change): The following cluster settings have been
renamed; the previous names remain available for
backward-compatibility.

| Previous name                         | New Name                                           |
|---------------------------------------|----------------------------------------------------|
| `server.shutdown.drain_wait`          | `server.shutdown.initial_wait`                     |
| `server.shutdown.lease_transfer_wait` | `server.shutdown.lease_transfer_iteration.timeout` |
| `server.shutdown.query_wait`          | `server.shutdown.queries.timeout`                  |
| `server.shutdown.connection_wait`     | `server.shutdown.transactions.timeout`             |
| `server.shutdown.jobs_wait`           | `server.shutdown.jobs.timeout`                     |
@knz
Copy link
Contributor Author

knz commented Oct 2, 2023

TFYR!

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Oct 2, 2023

Build succeeded:

@craig craig bot merged commit 0be8311 into cockroachdb:master Oct 2, 2023
3 checks passed
@knz knz deleted the 20230824-drain branch October 3, 2023 08:40
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.

sql, server: rename server.shutdown.lease_transfer_wait to server.shutdown.lease_transfer_timeout
3 participants