Skip to content

Conversation

@kamil-gwozdz
Copy link

@kamil-gwozdz kamil-gwozdz commented Aug 15, 2025

Description

This PR backports vitessio#16574.

Related Issue(s)

https://github.com/github/issues/issues/17729

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@kamil-gwozdz kamil-gwozdz changed the title patch-release-19.0-github backport https://github.com/vitessio/vitess/pull/16574 Aug 15, 2025
@kamil-gwozdz
Copy link
Author

"Backport to:" labels have been added if this change should be back-ported

I don't think we have any Backport to: labels in this repo. Is this applicable to my PR?

@mhamza15
Copy link

"Backport to:" labels have been added if this change should be back-ported

I don't think we have any Backport to: labels in this repo. Is this applicable to my PR?

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

@kamil-gwozdz
Copy link
Author

"Reparent Old Vtctl - Upgrade Downgrade Testing" is failing but it's also failing here so I guess it's okay

@kamil-gwozdz
Copy link
Author

unit_race tests failed but they are also failing in the same way on other PRs so im gonna assume my change is fine

@kamil-gwozdz kamil-gwozdz marked this pull request as ready for review August 20, 2025 10:18
Copilot AI review requested due to automatic review settings August 20, 2025 10:18
Copy link

Copilot AI left a 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_timeout from 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
Copy link

Copilot AI Aug 20, 2025

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.

Suggested change
github.com/prometheus/common v0.49.0 // indirect
github.com/prometheus/common v0.49.0

Copilot uses AI. Check for mistakes.
@kamil-gwozdz kamil-gwozdz requested a review from mhamza15 August 20, 2025 11:56
@kamil-gwozdz
Copy link
Author

@mhamza15 any chance you could help me roll this out?

@mhamza15
Copy link

@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?

@mhamza15
Copy link

v20 backport PR: #188

@kamil-gwozdz
Copy link
Author

Sounds good, thanks.

@kamil-gwozdz
Copy link
Author

closing because it's gonna be backported to v20 instead - #185 (comment)

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.

4 participants