release-22.2: cli: compute --drain-wait based on cluster setting values #98577
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Release note (cli change): The --drain-wait argument for the `drain` command will be automatically increased if the command detects that it is smaller than the sum of server.shutdown.drain_wait, server.shutdown.connection_wait, server.shutdown.query_wait times two, and server.shutdown.lease_transfer_wait. If the --drain-wait argument is 0, then no timeout is used. This recommendation was already documented, but now the advice will be applied automatically.
This fixes a minor bug in which the connection would not get closed at the earliest possible time during server shutdown. The connection is supposed to be closed as soon as we handle a Sync message when the conn_executor is in the draining state and not in a transaction. Since the transaction state was checked before state transitions occurred, this would cause the connection to remain open for an extra bit of time. This was particularly a problem because the Sync message is also the command that auto-commits an implicit transaction. So before this commit, it was actually impossible for the check to work as it was supposed to. Now we check the txn state after state transitions occur. Release note: None
|
This change is |
knz
changed the title
knz
approved these changes
Mar 14, 2023
The test now does much more: - Checks that --drain-wait is automatically increased if it is set lower than the cluster settings require. - Check that the /health?ready=1 endpoint fails during the drain_wait period. - Check for the proper error message during the connection_wait phase. - Check for the proper error message when trying to begin a new query/transaction during the query_wait phase. - Check that an open transaction is allowed to continue during the query_wait phase. - Check for the proper error message when a query is canceled during shutdown. Release note: None
rafiss
force-pushed
the
backport22.2-98390
branch
from
16c6175
to
4cb23c1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 3/3 commits from #98390.
/cc @cockroachdb/release
Release justification: low risk improvement to an important area
This includes a few commits related to draining
cli: compute --drain-wait based on cluster setting values
fixes #98388
Release note (cli change): The --drain-wait argument for the
draincommand will be automatically increased if the command detects that it is smaller than the sum of server.shutdown.drain_wait, server.shutdown.connection_wait, server.shutdown.query_wait times two, and server.shutdown.lease_transfer_wait.This recommendation was already documented, but now the advice will be applied automatically.
sql: fix check for closing connExecutor during draining
This fixes a minor bug in which the connection would not get closed at
the earliest possible time during server shutdown.
The connection is supposed to be closed as soon as we handle a Sync
message when the conn_executor is in the draining state and not in a
transaction. Since the transaction state was checked before state
transitions occurred, this would cause the connection to remain open for
an extra bit of time. This was particularly a problem because the Sync
message is also the command that auto-commits an implicit transaction.
So before this commit, it was actually impossible for the check to work
as it was supposed to.
Now we check the txn state after state transitions occur.
roachtest: enhance drain test
The test now does much more:
than the cluster settings require.
period.
query/transaction during the query_wait phase.
query_wait phase.
shutdown.