Skip to content

Make semi sync extension optional#5483

Merged
morgo merged 5 commits intovitessio:masterfrom
planetscale:morgo-semi-sync-optional
Dec 2, 2019
Merged

Make semi sync extension optional#5483
morgo merged 5 commits intovitessio:masterfrom
planetscale:morgo-semi-sync-optional

Conversation

@morgo
Copy link
Copy Markdown
Contributor

@morgo morgo commented Nov 27, 2019

Fixes #4762

There is a use case to attach Vitess to an existing MySQL topology and then migrate into Vitess. Currently this requires the semi sync extension to be present (irrespective of whether semi-sync is used). This is because of the way the mysql server handles config options: the plugins register them with the server. If the plugin is not present, there is an error because the option is unknown.

Since a user may be moving from a managed platform, or may not want to make too many changes to the source, it is important to not error if semi-sync commands can not be executed.

Signed-off-by: Morgan Tocker tocker@gmail.com

@morgo morgo requested a review from sougou as a code owner November 27, 2019 17:36
Signed-off-by: Morgan Tocker <tocker@gmail.com>
@morgo morgo force-pushed the morgo-semi-sync-optional branch from ee4f03c to 3abebd8 Compare November 27, 2019 20:26
…ptional

Signed-off-by: Morgan Tocker <tocker@gmail.com>
@morgo morgo force-pushed the morgo-semi-sync-optional branch from 342d1f9 to 959b626 Compare November 29, 2019 17:20
@morgo morgo requested a review from deepthi November 30, 2019 02:45
Copy link
Copy Markdown
Collaborator

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

There are other places where we are setting rpl_semi_sync variables. We need to evaluate whether changes are needed there. E.g.:
https://github.com/vitessio/vitess/blob/master/go/vt/mysqlctl/replication.go#L407

@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Dec 1, 2019

There are other places where we are setting rpl_semi_sync variables. We need to evaluate whether changes are needed there. E.g.:
https://github.com/vitessio/vitess/blob/master/go/vt/mysqlctl/replication.go#L407

I actually think that SetSemiSyncEnabled behaves correctly without modification. i.e. if you try and enable semi-sync but the plugin is not loaded, the error is caught.

But there is another one called SemiSyncEnabled which could benefit. It currently does something very similar to the function I introduce:

https://github.com/vitessio/vitess/blob/master/go/vt/mysqlctl/replication.go#L419-L422

If I modified this to first use SemisyncExtensionLoaded, it would be one extra query though, since it needs the output of the query when the extension is loaded.

morgo added 3 commits December 1, 2019 09:10
…ptional

Signed-off-by: Morgan Tocker <tocker@gmail.com>
…ess into morgo-semi-sync-optional

Signed-off-by: Morgan Tocker <tocker@gmail.com>
…ptional

Signed-off-by: Morgan Tocker <tocker@gmail.com>
@morgo
Copy link
Copy Markdown
Contributor Author

morgo commented Dec 1, 2019

I renamed SemisyncExtensionLoaded to SemiSyncExtensionLoaded to be consistent with existing code.

@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Dec 2, 2019

But there is another one called SemiSyncEnabled which could benefit. It currently does something very similar to the function I introduce:

https://github.com/vitessio/vitess/blob/master/go/vt/mysqlctl/replication.go#L419-L422

If I modified this to first use SemisyncExtensionLoaded, it would be one extra query though, since it needs the output of the query when the extension is loaded.

I don't think that is needed, since it will return false if the extension is not loaded (catch error and return false).

Copy link
Copy Markdown
Collaborator

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM

@morgo morgo merged commit 9fa3087 into vitessio:master Dec 2, 2019
@morgo morgo deleted the morgo-semi-sync-optional branch December 2, 2019 17:08
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.

InitShardMaster requires the rpl_semi_sync_master and repl_semi_sync_slave plugins are installed

2 participants