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

Gracefully shutdown VTGate instances #14219

Merged
merged 4 commits into from
Oct 23, 2023

Conversation

davidpiegza
Copy link
Contributor

@davidpiegza davidpiegza commented Oct 10, 2023

Description

Currently, during a VTGate shutdown it's possible for established connections to execute new queries, those queries will fail with vttablet: Connection Closed errors (see also this comment).

This PR adds a check in MySQL server plugin if the connection is shutting down and returns a ERServerShutdown error.

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on the CI
  • Documentation was added or is not required

Deployment Notes

Signed-off-by: David Piegza <davidpiegza@github.com>
@github-actions github-actions bot added this to the v19.0.0 milestone Oct 10, 2023
@arthurschreiber arthurschreiber added Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Oct 10, 2023
Signed-off-by: David Piegza <davidpiegza@github.com>

func GetTestServerConn(listener *Listener) *Conn {
return newServerConn(testConn{}, listener)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this function (and GetTestConn above it) be moved to conn_flaky_test.go instead? So it's in a proper _test.go file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to move it but it's not able find it then. Could it be that functions in _test.go are not exported?

Copy link
Member

Choose a reason for hiding this comment

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

Yes functions in test files can only be used locally. They aren't exported.

From the Internet -

Files matching *_test.go are only compiled when testing the package they are part of. If you are testing package A that uses package B, you won't have access to the _test.go code from package B.

Copy link
Member

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

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

Rest LGTM!

go/mysql/conn.go Outdated
// GetTestConn returns a conn for testing purpose only.
func GetTestConn() *Conn {
return newConn(testConn{})
}

func GetTestServerConn(listener *Listener) *Conn {
Copy link
Member

Choose a reason for hiding this comment

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

I know the function already says Test but maybe it is a good idea to add a comment something like -

// GetTestServerConn is only meant to be used for testing. It creates a server connection using a testConn and the provided listener.

And I would move this function to the conn_fake.go file. That files defines the testConn struct too and it contains all the test code we keep in the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GuptaManan100 Thanks! I moved this and the other GetTestConn() function to conn_fake.go.

Signed-off-by: David Piegza <davidpiegza@github.com>
…in-vtgate

Signed-off-by: David Piegza <davidpiegza@github.com>
@davidpiegza
Copy link
Contributor Author

Would it be possible to backport this to latest v18 as well (after release)?

@frouioui
Copy link
Member

Would it be possible to backport this to latest v18 as well (after release)?

We usually don't backport enhancements/new features to previous branches. Are you able to do a custom build with this new feature?

@frouioui frouioui merged commit ad26d15 into vitessio:main Oct 23, 2023
115 checks passed
brendar pushed a commit to Shopify/vitess that referenced this pull request Oct 1, 2024
Signed-off-by: Brendan Dougherty <brendan.dougherty@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants