mysqlctld: setup a different default for onterm_timeout #15575
Merged
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
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
Until now we've had the same timeouts for shutdown etc. across the different binaries. This doesn't provide the necessary flexibility though, since for example `mysqlctld` will need longer timeouts. This is because it needs to shut down MySQL and we need it to honor the MySQL shutdown timeout as well. This means this can take significantly longer to ensure the shutdown is clean. It's necessary to do this, to ensure things like being able to upgrade MySQL since that depends on clean shutdowns. Signed-off-by: Dirkjan Bussink <[email protected]>
dbussink
force-pushed
the
dbussink/longer-mysqctld-term-timeout
branch
from
96a854f to
e45438b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15575 +/- ##
==========================================
- Coverage 67.41% 65.75% -1.67%
==========================================
Files 1560 1561 +1
Lines 192752 194851 +2099
==========================================
- Hits 129952 128129 -1823
- Misses 62800 66722 +3922 ☔ View full report in Codecov by Sentry. |
dbussink
changed the title
deepthi
removed
NeedsDescriptionUpdate
deepthi
reviewed
Mar 26, 2024
There was a problem hiding this comment.
I am onboard with this change. However it is technically a backwards incompatible change, so we need to document it as such.
Let's create an issue and write a release note. I believe website flag docs will also need to be regenerated unless that is now automatically triggered by merges on the main repo.
deepthi
added
the
release notes (needs details)
Signed-off-by: Dirkjan Bussink <[email protected]>
dbussink
requested review from
rohit-nayak-ps,
shlomi-noach and
mattlord
as code owners
deepthi
reviewed
Mar 26, 2024
There was a problem hiding this comment.
LGTM. Let's get the issue created and release note added, and I can approve after I review the release note.
For the record, I think it is indeed a good idea to start moving globals at the servenv level into structs that can be overridden by each binary, so thank you for starting us down this path.
| @@ -87,7 +87,7 @@ var ( | |||
| mysqlPort = 3306 | |||
| mysqlSocket string | |||
| mysqlTimeout = 5 * time.Minute | |||
| mysqlShutdownTimeout = 5 * time.Minute | |||
| mysqlShutdownTimeout = mysqlctl.DefaultShutdownTimeout | |||
There was a problem hiding this comment.
Insane that we were defining this 5 minutes in multiple places.
deepthi
changed the title
They are generated by vitess-bot, so I've removed the label for website docs. |
Signed-off-by: Dirkjan Bussink <[email protected]>
dbussink
added
Type: Bug
Component: TabletManager
and removed
NeedsIssue
|
@deepthi Added a release note as well. |
dbussink
removed
the
release notes (needs details)
frouioui
approved these changes
Mar 26, 2024
deepthi
approved these changes
Mar 26, 2024
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.
Until now we've had the same timeouts for shutdown etc. across the different binaries. This doesn't provide the necessary flexibility though, since for example
mysqlctldwill need longer timeouts.This is because it needs to shut down MySQL and we need it to honor the MySQL shutdown timeout as well. This means this can take significantly longer to ensure the shutdown is clean.
It's necessary to do this, to ensure things like being able to upgrade MySQL since that depends on clean shutdowns.
Related Issue(s)
Fixes #15577
Checklist