-
Notifications
You must be signed in to change notification settings - Fork 12
backport https://github.com/vitessio/vitess/pull/16574 #185
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
Conversation
Signed-off-by: Harshit Gangal <[email protected]>
I don't think we have any |
These are all artifacts from upstream. That's how they handle backporting new changes. For the most part, ignore everything in this PR 😅 . Just make sure CI passes |
|
"Reparent Old Vtctl - Upgrade Downgrade Testing" is failing but it's also failing here so I guess it's okay |
|
unit_race tests failed but they are also failing in the same way on other PRs so im gonna assume my change is fine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR backports functionality related to handling the innodb_lock_wait_timeout system variable in Vitess, allowing it to be set and accessed at both session and global scopes.
- Moves
innodb_lock_wait_timeoutfrom read-only to settable system variables - Updates AST rewriting to properly handle global scope variables by not rewriting them
- Adds comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| go/vt/vtgate/executor_set_test.go | Adds test cases for setting innodb_lock_wait_timeout at session and global scopes |
| go/vt/vtgate/executor_select_test.go | Adds comprehensive test for global and session variable behavior |
| go/vt/sysvars/sysvars.go | Moves innodb_lock_wait_timeout from read-only to settable variables list |
| go/vt/sqlparser/ast_rewriting.go | Updates variable rewriting logic to exclude global scope variables |
| go/test/endtoend/vtgate/reservedconn/sysvar_test.go | Adds end-to-end test for innodb_lock_wait_timeout functionality |
| go.mod | Adds indirect comment to prometheus/common dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| github.com/planetscale/vtprotobuf v0.5.0 | ||
| github.com/prometheus/client_golang v1.19.0 | ||
| github.com/prometheus/common v0.49.0 | ||
| github.com/prometheus/common v0.49.0 // indirect |
Copilot
AI
Aug 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding an // indirect comment to an existing dependency without actually making it indirect could be misleading. This change appears unrelated to the PR's purpose of backporting innodb_lock_wait_timeout functionality.
| github.com/prometheus/common v0.49.0 // indirect | |
| github.com/prometheus/common v0.49.0 |
|
@mhamza15 any chance you could help me roll this out? |
|
@kamil-gwozdz Rolling out any change across the fleet is somewhat blocked due to an issue that should be fixed by this upstream PR: vitessio#18530. We are planning on backporting it along with the v20 upgrade. I think our way forward is to include this change with v20 and roll it out along with the upgrade. The upgrade is targeted to complete by 9/12 (tracking issue). Does that sound good with y'all? |
|
v20 backport PR: #188 |
|
Sounds good, thanks. |
|
closing because it's gonna be backported to v20 instead - #185 (comment) |
Description
This PR backports vitessio#16574.
Related Issue(s)
https://github.com/github/issues/issues/17729
Checklist
Deployment Notes