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

storage/mysql: Replace enable_mysql_source with max_mysql_connections var #27847

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

rjobanp
Copy link
Contributor

@rjobanp rjobanp commented Jun 24, 2024

Motivation

Parity with other source types, and the ability to query this variable from the Console (via SQL). See https://materializeinc.slack.com/archives/C01CFKM1QRF/p1715228421858309?thread_ts=1715209780.830859&cid=C01CFKM1QRF for more context

Tips for reviewer

Checklist

@rjobanp rjobanp requested a review from a team June 24, 2024 15:32
@rjobanp rjobanp requested review from a team as code owners June 24, 2024 15:32
@rjobanp rjobanp requested a review from maddyblue June 24, 2024 15:32
Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

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

I triggered nightly just in case this has some weird side effects with upgrades: https://buildkite.com/materialize/nightly/builds/8207

Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

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

There is a bunch of failures in nightly, but they should all be fixable by bumping the limit. Ping me if you want me to fix those errors as part of this PR.

@@ -432,6 +432,13 @@ pub static MAX_POSTGRES_CONNECTIONS: VarDefinition = VarDefinition::new(
false,
);

pub static MAX_MYSQL_CONNECTIONS: VarDefinition = VarDefinition::new(
"max_mysql_connections",
value!(u32; 0),
Copy link
Member

Choose a reason for hiding this comment

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

Your life will probably be much easier if you default this to 1000, and then use LD in production to set it back to 0 by default for production environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah great idea - will do

Copy link
Contributor Author

@rjobanp rjobanp Jun 24, 2024

Choose a reason for hiding this comment

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

Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

Awesome, thank you, Roshan!

@rjobanp rjobanp force-pushed the mysql-connection-limit branch 4 times, most recently from cff8c90 to 93ec7b8 Compare June 24, 2024 20:50
@rjobanp rjobanp merged commit e1a3913 into MaterializeInc:main Jun 25, 2024
74 checks passed
@rjobanp rjobanp deleted the mysql-connection-limit branch June 25, 2024 18:38
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.

3 participants