Fix: convertBoolToSemiSyncAction method to account for all semi sync actions

[WilliamLu99]

Description

Fixes a bug where ChangeTabletType would fail on clusters that don't have rpl_semi_sync_master and rpl_semi_sync_slave plugins loaded. Does so by refactoring the convertBoolToSemiSyncAction method to return SemiSyncActionNone if the plugin is not loaded. To do this, we query the underlying mysql to check if the rpl_semi_sync_% variables are present.

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

[WilliamLu99]

Not actually going to merge this, just creating the PR here first to get some feedback internally, check that CI passes. Then will try to upstream the change.

[hkdsun]

An improvement would be to check the INFORMATION_SCHEMA.PLUGINS meta table to check for extension being loaded. It is more performant (showing global variables is an expensive operation) and another reason being that, according to Jeremy, all SHOW ... queries are deprecated.

Here's a query you can plug in easily:

mysql> select count(*) > 0 as plugin_loaded from information_schema.plugins where plugin_name like "rpl_semi_sync_%";
+---------------+
| plugin_loaded |
+---------------+
|             0 |
+---------------+
1 row in set (0.00 sec)

plugin_loaded will be either 0 or 1

See:

https://dev.mysql.com/doc/refman/8.0/en/information-schema-plugins-table.html
and
https://dev.mysql.com/doc/refman/8.0/en/replication-semisync-installation.html

[hkdsun]

Once we've improved the query to check for the extension being loaded, I think this can be proposed upstream!

[WilliamLu99]

I rebased to main and now the replicas start with super_read_only, which forbids us to uninstall plugins.

[hkdsun]

Looks great!! I think this is definitely ready to be released/staged as a Shopify hotfix

In terms of submitting upstream, this is a nitpick but I think the level of abstraction of the test (end-to-end testing ChangeTabletType) doesn't match the level of abstraction of the change itself very well (how we interpret a false value).

[hkdsun]

If we are submitting upstream, I must say that it's a bit weird to have this be the only test. Maybe we could add a unit test (on the tablet manager) when submitting?

[WilliamLu99]

Fair point, I'll add an unit test.

[WilliamLu99]

Took a look again. I don't think a unit test would fit in this scenario, as there's not much to unit test. Unit tests in this package use the fakemysqlddaemon to mock the Mysqld type that has the SemiSyncExtensionLoaded call.

Because of that, it limits the ability of what we are testing. A unit test in mysqlctl won't actually help us test any of the changes we made.

Fakemysqldaemon also mocks a bunch of other semi sync related methods that would otherwise break without my change.

Unit tests within this codebase in general seem to abstract away interactions with the DB. I think the endtoend cluster test is the right place for our tests, and that this test offers sufficient coverage for this change. I'll push this upstream, and if I get any feedback related to testing I'll make the change then.

[arthurschreiber]

👋 Hey friends!

We've run into the same issue as well. If there's anything we can do to help get this upstreamed, please let me know. 🙇‍♂️

[hkdsun]

Hey Arthur, thanks for stopping by our fork. The upstream PR for this has been approved but William is digging into some suspect test failures here: vitessio#12914

[github-actions]

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

• Push additional commits to the associated branch.
• Remove the stale label.
• Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

[github-actions]

This PR was closed because it has been stale for 7 days with no activity.