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

ccl/sqlproxyccl: add --disable-connection-rebalancing flag to "mt start-proxy" #81712

Conversation

jaylim-crl
Copy link
Collaborator

@jaylim-crl jaylim-crl commented May 24, 2022

Previously, we added the connection rebalancing feature to the proxy, and that
gets enabled automatically. This feature will not work with < v22.1 clusters
since it relies on the session migration work that was added recently. In
theory, the proxy will still work, but the balancer will constantly attempt
to rebalance connection when we know that it will fail, and this adds to the
overall latency of the connection since we suspend the processors during
connection migration.

To allow us to transition to v22.1 nicely, we will introduce a new flag
--disable-connection-rebalancing to the mt start-proxy subcommand. When
that flag is set, all connection rebalancing operations will be disabled. We
have to roll out sqlproxy to CC before the clusters get their major upgrade
since it contains some auth work that is a pre-req to v22.1. To avoid these
latency issues, sqlproxy will be rolled out with the flag set. Once clusters
have been upgraded to v22.1, the flag will be removed during startup.

No release notes since mt start-proxy is internal only.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jaylim-crl jaylim-crl marked this pull request as ready for review May 24, 2022 03:59
@jaylim-crl jaylim-crl requested review from a team as code owners May 24, 2022 03:59
@jaylim-crl jaylim-crl requested review from jeffswenson and andy-kimball and removed request for a team May 24, 2022 03:59
@jaylim-crl jaylim-crl force-pushed the jay/220523-add-flag-to-disable-connection-migration branch from d9cd819 to 2d226ad Compare May 24, 2022 14:29
@jaylim-crl
Copy link
Collaborator Author

Strange. Couldn't seem to get the bazel tests to pass, but they all pass locally:

INFO: Build completed successfully, 401 total actions
//pkg/ccl/sqlproxyccl:sqlproxyccl_test                          (cached) PASSED in 45.2s
//pkg/ccl/sqlproxyccl/balancer:balancer_test                             PASSED in 5.9s
//pkg/ccl/sqlproxyccl/denylist:denylist_test                             PASSED in 1.8s
//pkg/ccl/sqlproxyccl/interceptor:interceptor_test                       PASSED in 0.6s
//pkg/ccl/sqlproxyccl/tenant:tenant_test                                 PASSED in 10.9s
//pkg/ccl/sqlproxyccl/throttler:throttler_test                           PASSED in 0.3s

@jaylim-crl jaylim-crl force-pushed the jay/220523-add-flag-to-disable-connection-migration branch 2 times, most recently from a7a0a34 to 7b91a21 Compare May 25, 2022 00:51
@jaylim-crl jaylim-crl marked this pull request as draft May 25, 2022 00:51
@jaylim-crl
Copy link
Collaborator Author

Making this a draft to figure out what went wrong with the bazel tests.

@jaylim-crl jaylim-crl force-pushed the jay/220523-add-flag-to-disable-connection-migration branch 7 times, most recently from c0fc1f5 to 04eafbf Compare May 25, 2022 21:47
…rt-proxy"

Previously, we added the connection rebalancing feature to the proxy, and that
gets enabled automatically. This feature will not work with < v22.1 clusters
since it relies on the session migration work that was added recently. In
theory, the proxy will still work, but the balancer will constantly attempt
to rebalance connection when we know that it will fail, and this adds to the
overall latency of the connection since we suspend the processors during
connection migration.

To allow us to transition to v22.1 nicely, we will introduce a new flag
`--disable-connection-rebalancing` to the `mt start-proxy` subcommand. When
that flag is set, all connection rebalancing operations will be disabled. We
have to roll out sqlproxy to CC before the clusters get their major upgrade
since it contains some auth work that is a pre-req to v22.1. To avoid these
latency issues, sqlproxy will be rolled out with the flag set. Once clusters
have been upgraded to v22.1, the flag will be removed during startup.

No release notes since `mt start-proxy` is internal only.

Release note: None
@jaylim-crl jaylim-crl force-pushed the jay/220523-add-flag-to-disable-connection-migration branch from 04eafbf to 0fcb822 Compare May 25, 2022 21:54
@jaylim-crl
Copy link
Collaborator Author

It turned out that we had to bump the bazel test size from small to medium because we're hitting our timeout threshold. Hopefully tests are green this time.

@jaylim-crl jaylim-crl marked this pull request as ready for review May 25, 2022 21:56
Copy link
Collaborator

@jeffswenson jeffswenson left a comment

Choose a reason for hiding this comment

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

LGTM

@jaylim-crl
Copy link
Collaborator Author

Flaked on pkg/sql/logictest, which is unrelated to this PR.

bors r=JeffSwenson

@jaylim-crl
Copy link
Collaborator Author

TFTR!

@craig
Copy link
Contributor

craig bot commented May 26, 2022

Build succeeded:

@craig craig bot merged commit e4c7ca5 into cockroachdb:master May 26, 2022
@jaylim-crl jaylim-crl deleted the jay/220523-add-flag-to-disable-connection-migration branch November 30, 2022 16:55
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