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

Switch to use semisync source / replica plugins #15791

Merged

Conversation

dbussink
Copy link
Contributor

@dbussink dbussink commented Apr 25, 2024

The plugins with the old terminology are deprecated and will be removed in the future. In MySQL 8.4.0 also the old terminology for regular replication is removed.

This starts the move to use the new style. Since MySQL 8.0.26 the new semisync plugin is available, so we should start using it from that version on.

This means we also need new default MySQL cnf files to set up things correctly. We add here a mysql8026.cnf and also a pre-emptive mysql84.cnf. The latter is needed to start the work in the future for MySQL 8.4.0. The main change here is that the deprecated mysql_native_password plugin needs to be explicitly enabled.

Removing our usage of mysql_native_password is another separate significant effort. The main issue there is how we want to deal with certificates etc. which end up being required for replication if you want to use caching_sha2_password which adds a whole layer of complexity we've never had to deal with.

Related Issue(s)

Part of #11716

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Copy link
Contributor

vitess-bot bot commented Apr 25, 2024

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Apr 25, 2024
@github-actions github-actions bot added this to the v20.0.0 milestone Apr 25, 2024
Copy link

codecov bot commented Apr 25, 2024

Codecov Report

Attention: Patch coverage is 50.65789% with 75 lines in your changes are missing coverage. Please review.

Project coverage is 68.44%. Comparing base (ee6b837) to head (500a31f).

Files Patch % Lines
go/vt/mysqlctl/replication.go 68.00% 24 Missing ⚠️
go/vt/vttablet/tabletmanager/rpc_replication.go 18.51% 22 Missing ⚠️
go/vt/mysqlctl/mysqld.go 0.00% 6 Missing ⚠️
go/cmd/vtcombo/cli/main.go 0.00% 5 Missing ⚠️
go/mysql/flavor_mysql.go 37.50% 5 Missing ⚠️
go/vt/mysqlctl/fakemysqldaemon.go 50.00% 4 Missing ⚠️
go/vt/vttablet/tabletmanager/rpc_actions.go 57.14% 3 Missing ⚠️
go/mysql/flavor_mariadb.go 0.00% 2 Missing ⚠️
go/mysql/replication.go 77.77% 2 Missing ⚠️
go/vt/mysqlctl/builtinbackupengine.go 50.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15791   +/-   ##
=======================================
  Coverage   68.43%   68.44%           
=======================================
  Files        1558     1558           
  Lines      195971   196036   +65     
=======================================
+ Hits       134121   134169   +48     
- Misses      61850    61867   +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dbussink dbussink force-pushed the dbussink/use-semisync-source-replica branch 7 times, most recently from cd39637 to d860bb2 Compare April 25, 2024 13:36
@dbussink dbussink added Type: Internal Cleanup Component: General Changes throughout the code base and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Apr 25, 2024
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.

Looks good to me!

go/mysql/flavor_mariadb.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/replication.go Outdated Show resolved Hide resolved
go/vt/mysqlctl/replication.go Outdated Show resolved Hide resolved
@deepthi
Copy link
Member

deepthi commented Apr 25, 2024

Nice work. I like that you handled some of the tech-debt around not passing a context to some of these functions too.

@dbussink dbussink force-pushed the dbussink/use-semisync-source-replica branch from d860bb2 to bc178bb Compare April 25, 2024 18:43
The plugins with the old terminology are deprecated and will be removed
in the future. In MySQL 8.4.0 also the old terminology for regular
replication is removed.

This starts the move to use the new style. Since MySQL 8.0.26 the new
semisync plugin is available, so we should start using it from that
version on.

This means we also need new default MySQL cnf files to set up things
correctly. We add here a mysql8026.cnf and also a pre-emptive
mysql84.cnf. The latter is needed to start the work in the future for
MySQL 8.4.0. The main change here is that the deprecated
mysql_native_password plugin needs to be explicitly enabled.

Removing our usage of mysql_native_password is another separate
significant effort. The main issue there is how we want to deal with
certificates etc. which end up being required for replication if you
want to use caching_sha2_password which adds a whole layer of
complexity we've never had to deal with.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
@dbussink dbussink force-pushed the dbussink/use-semisync-source-replica branch from bc178bb to a2342d8 Compare April 25, 2024 19:11
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
@dbussink dbussink merged commit 8f6cfaa into vitessio:main Apr 26, 2024
104 checks passed
@dbussink dbussink deleted the dbussink/use-semisync-source-replica branch April 26, 2024 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: General Changes throughout the code base Type: Internal Cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants