Add missing options to CHANGE MASTER TO

[Jorge-Rodriguez]

SUMMARY

Added all missing options for CHANGE MASTER TO, according to https://dev.mysql.com/doc/refman/8.0/en/change-master-to.html

Fixes #59

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

mysql_replication

[Jorge-Rodriguez]

@Andersson007 @bmalynovytch @bmildren I'm not quite sure how to write tests for this, other than running the module with the new options and check that the sent query matches expectation and the server doesn't spit an error. Do you have any suggestions?

[codecov[bot]]

Codecov Report

Merging #63 (00fa058) into main (00fa058) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 00fa058 differs from pull request most recent head f07e389. Consider uploading reports for the commit f07e389 to get more accurate results

@@           Coverage Diff           @@
##             main      #63   +/-   ##
=======================================
  Coverage   77.88%   77.88%           
=======================================
  Files          28       28           
  Lines        2347     2347           
  Branches      552      552           
=======================================
  Hits         1828     1828           
  Misses        362      362           
  Partials      157      157           

Flag	Coverage Δ
integration	75.05% <0.00%> (ø)
sanity	15.30% <0.00%> (ø)
units	32.16% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Jorge-Rodriguez]

@Andersson007 the sanity checks say I can't define choices for an integer parameter. What's the proper way to define an int within a specific range of values?

[Andersson007]

@Andersson007 the sanity checks say I can't define choices for an integer parameter. What's the proper way to define an int within a specific range of values?

I've never seen int ranges, there aren't a lot to list the whole sequence, i.e. [1, 2, ...] , or maybe we could just document this and check in the code if a passed value is in the range.

[Andersson007]

@Andersson007 @bmalynovytch @bmildren I'm not quite sure how to write tests for this, other than running the module with the new options and check that the sent query matches expectation and the server doesn't spit an error. Do you have any suggestions?

Can we run the 3d instance? As i can see there are mysql_primary_port and mysql_replica1_port there now. Maybe to run one more as a replica with mysql_replica2_port and switch it to mysql_replica1_port as a master. I'm not sure if it works with MySQL but in Postgres we can use a "cascade" of replicas and switch between them so that one will become a master to another being a replica of another one without rebuilding. If it's complicated, your suggestion, imo, also works.

but @bmildren and @bmalynovytch , as experts, know better:)

[bmalynovytch]

Absolutely, you may have 1 master with 2 slaves and switch one of the slaves to master and attach the second one as new slave of the new master.

[Jorge-Rodriguez]

@bmalynovytch since all of the options to CHANGE MASTER TO are optional and we aren't testing the MASTER_HOST or MASTER_PORT options (those are already tested) couldn't we test everything against the running master?

The other, more complicated question is what kind of effects to look for to verify that the command succeeded. But in that case I would assume that if the query sent to the server matches the expectation and the server doesn't err then the server has done as requested. It could be the case that there server remains silent from the module's PoV but didn't honour the request, but my knowledge of mysql replication is not deep enough to foresee these cases.

[Jorge-Rodriguez]

@Andersson007 the sanity checks say I can't define choices for an integer parameter. What's the proper way to define an int within a specific range of values?

I've never seen int ranges, there aren't a lot to list the whole sequence, i.e. [1, 2, ...] , or maybe we could just document this and check in the code if a passed value is in the range.

Maybe we can define it as a list of string representations of integers. At the end of the day however the value is written in YAML didn't make that much of a difference, and we end up turning it into a string for the cursor query anyway.

[Andersson007]

@Andersson007 the sanity checks say I can't define choices for an integer parameter. What's the proper way to define an int within a specific range of values?

I've never seen int ranges, there aren't a lot to list the whole sequence, i.e. [1, 2, ...] , or maybe we could just document this and check in the code if a passed value is in the range.

Maybe we can define it as a list of string representations of integers. At the end of the day however the value is written in YAML didn't make that much of a difference, and we end up turning it into a string for the cursor query anyway.

For me personally the best approach is documenting the acceptable range, removing choices, and checking on the passed value in the code with raising a helpful message if the value is out of the borders.

[Jorge-Rodriguez]

I'm going to abandon this PR draft, as it relates to replication and we have modifications regarding that component and the language used. That being said, I'd like to keep this PR still open for reference until I get the chance to tackle the issue again.

[Andersson007]

@Jorge-Rodriguez sure, it's marked as DRAFT, so, people will be warned. Maybe also would be good to put something like "TEMPORARY FROZEN" to the description.

[Andersson007]

@Jorge-Rodriguez any updates on the PR?

[Jorge-Rodriguez]

I haven't gotten the chance to work on this for quite some time. I'll pick it up, seems there's only the "enum" issue to solve and to write appropriate tests.

Thanks for the ping.

[Jorge-Rodriguez]

@Andersson007 I have just brought the code of this PR back to date. Still pending changes to the "enum" issue and testing, so I'm not moving it from draft status yet.

[Jorge-Rodriguez]

I am unable to run integration tests locally, When running on docker the first get_url task fails with HTTP 403. The URL is otherwise OK and it works, so it would seem an issue with docker?

[Jorge-Rodriguez]

I found out that all our tests were run with the install_mariadb flag set to true.

[Jorge-Rodriguez]

Tests are broken because downloading the MySQL tarball consistently fails with HTTP 403. The reason is unknown as of this writing.

[Jorge-Rodriguez]

It seems that the old dev.mysql.com URL is not working anymore and the resources have been moved to cdn.mysql.com. For some reason, the request from the get_url module does not get redirected.

[Andersson007]

@Jorge-Rodriguez great work, thank you! And thanks for investigating the reason of 403.

1. Would you like to raise a separate PR for the URL change you did here?
2. Do you think we need those master/slave aliases? I think we left them for older options not to break stuff in users WFs. As these are new options, not sure if we should add the aliases. Ideas?

[Jorge-Rodriguez]

Yeah, it might be a good idea not to introduce futile backwards compatibility on new features, I'll remove the aliases.

[Andersson007]

@Jorge-Rodriguez thanks! please let me know when it's ready for review. There are a lot of parameters. Are they all supported by all MySQL/MariaDB supported versions? If not, i'm not sure if we should document minimal supporting versions for each new argument or not? Maybe it's an extra thing. Ideas?

[Jorge-Rodriguez]

I've just rebased the whole thing to make the commit history clearer. I'm still keeping the CI fixup so the tests run correctly, I'll remove them when #491 is merged.

@Andersson007 no need for review just yet. I still have to write tests for the new parameters, I'm going to keep the PR as a draft until those are ready.

[Jorge-Rodriguez]

@Andersson007 What's the proper way to document an option that is only available since a specific version of MySQL?

[Andersson007]

@Andersson007 What's the proper way to document an option that is only available since a specific version of MySQL?

@Jorge-Rodriguez imo just to add another row to each option saying something like - Available (or supported maybe) since MySQL <version > / MariaDB <version> would be enough.

@laurent-indermuehle @betanummeric @bmalynovytch other thoughts ^ ?

[laurent-indermuehle]

1. If used on older version, will it be ignored or crash?
2. if used on another engine (mysql vs mariadb), will it be ignored or crash?

I hope that 'Plugins CI' will tell us once it runs ;)

[Andersson007]

1. If used on older version, will it be ignored or crash?
2. if used on another engine (mysql vs mariadb), will it be ignored or crash?

I hope that 'Plugins CI' will tell us once it runs ;)

I believe just crashing is OK as soon as the versions are documented. We, of course, can check versions in our code, etc. but shouldn't this be user's responsibility? If a user runs unsupported SQL, RDBMS just shows errors (invalid syntax or something), doesn't it?

[laurent-indermuehle]

I believe just crashing is OK as soon as the versions are documented. We, of course, can check versions in our code, etc. but shouldn't this be user's responsibility? If a user runs unsupported SQL, RDBMS just shows errors (invalid syntax or something), doesn't it? Right, ansible-doc will show the user that we documented the minimum version and his logs will tell him what went wrong.

I searched the repo and found many occurences:

• Supported from MariaDB 10.0.1.
• Multi-source replication is supported from MySQL 5.7.
• Roles are supported since MySQL 8.0.0 and MariaDB 10.0.5.
• Supported by B(MariaDB).
• Is not supported by MariaDB and is silently ignored when working with MariaDB.
• Supported by MySQL 8.0 or later.

English is not my first language, so I don't know what the most correct form is. But I know I don't like seen a dot at the end of the version.

[Jorge-Rodriguez]

I'm planning to spot the problematic options as I progress with test writing and have the module fail gracefully with an appropriate message.

[Jorge-Rodriguez]

In a different order of things, it seems that since we started making modificaciones to the replication module, MySQL has settled on replacing the master term with source instead of our chosen primary so while I'm at this I'm going to introduce that rename too

[Jorge-Rodriguez]

1. If used on older version, will it be ignored or crash?
2. if used on another engine (mysql vs mariadb), will it be ignored or crash?

I hope that 'Plugins CI' will tell us once it runs ;)

I believe just crashing is OK as soon as the versions are documented. We, of course, can check versions in our code, etc. but shouldn't this be user's responsibility? If a user runs unsupported SQL, RDBMS just shows errors (invalid syntax or something), doesn't it?

The problem with delegating the error to the connector is that it can become very unclear. As a user you'd get a message such as

[...] PLEASE CHECK YOUR SYNTAX NEAR [...]

But the user didn't construct that query, the module did, so it would require intimate knowledge of the module's inner workings to determine what is causing the problem.

IMO it'd be better to have the module do the version checking and fail with a proper message like

You're using DB version A and argument 'you_jumped_the_gun' isn't supported until version B.