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

master/slave → primary/replica #18102

Merged
merged 2 commits into from Jun 13, 2020
Merged

Conversation

@brandonkelly
Copy link
Contributor

brandonkelly commented Jun 13, 2020

There is a growing call for the tech industry to move away from “master”/“slave” terminology in the interest of inclusion, especially toward the Black community.

In that spirit, this PR deprecates those terms as they relate to read/write splitting, in favor of “primary” and “replica” – which I’d argue are not just more inclusive terms, but also more semantically correct. (The replica servers are not merely “taking orders” from their “master” server; they are replicating it.)

I’ve managed to do this in a non-breaking way, by keeping old master/slave methods around as aliases for the new ones, and adding magic getter/setter methods for the old master/slave properties. I’ve also introduced a couple traits to document the now-deprecated properties, based on https://stackoverflow.com/a/51890536/1688568.

Code that demonstrates how PhpStorm recognizes yii\db\Connection::$master as deprecated

Q A
Is bugfix?
New feature?
Breaks BC?
Tests pass? ✔️
@brandonkelly brandonkelly marked this pull request as ready for review Jun 13, 2020
@brandonkelly brandonkelly force-pushed the brandonkelly:primary-replica branch from e77ea68 to 3fe0df1 Jun 13, 2020
@rob006
Copy link
Member

rob006 commented Jun 13, 2020

Master-slave is common naming pattern in this case. You may consider new term more inclusive and correct, but i is also more confusing if you want to read and refer to any existing documentation.
https://mariadb.com/resources/blog/database-master-slave-replication-in-the-cloud/
https://en.wikipedia.org/wiki/Master/slave_(technology)

Also, I don't think that "replica" is correct term here and it does not mean the same as "slave" in "master/slave" model.

@brandonkelly
Copy link
Contributor Author

brandonkelly commented Jun 13, 2020

I chose those terms because SQL Server and RDS use them:

  • SQL Server - Configure replication:

    …If set to 1 Windows authentication is used to connect to the current primary. If set to 0, SQL Server authentication is used with the specified @login and @password values. The login and password specified must be valid at each secondary replica for the validation stored procedure to successfully connect to that replica

  • RDS - Working with Read Replicas

    …Business reporting or data warehousing scenarios where you might want business reporting queries to run against a read replica, rather than your primary, production DB instance…

Also:

“Master”/“slave” may still be in use by some DBs, but it’s on its way out, and “replica” is the new conventional term.

@brandonkelly
Copy link
Contributor Author

brandonkelly commented Jun 13, 2020

Also, from the very Wikipedia article you linked to:

Terminology concerns

The terminology has sometimes been replaced with something different because the terms "master" and "slave" refer to the practice of slavery.

"Main" & "Secondary" has been proposed, enabling reusable interpretation of the remaining acronym labels for connecting.

One alternative for databases is "primary" and "replica", which is used in the documentation from IBM, Microsoft, Engine Yard, Amazon Web Services/Amazon Relational Database Service, and ACM as well as in Python, Django, Drupal, CouchDB, Redis and MediaWiki (which still uses "master").

@Thoulah
Copy link
Contributor

Thoulah commented Jun 13, 2020

Are you gonna rename whitelist / blacklist too?

@onebrightlight
Copy link

onebrightlight commented Jun 13, 2020

Are you gonna rename whitelist / blacklist too?

That sounds like a great suggestion too, tbh.

@brandonkelly
Copy link
Contributor Author

brandonkelly commented Jun 13, 2020

It crossed my mind but Yii doesn’t have explicit “whitelist”/“blacklist” concepts. The words only show up in a couple comments/docs. Can look into updating those in a separate PR, but this felt more pressing as a starting point.

@samdark samdark added this to the 2.0.36 milestone Jun 13, 2020
Copy link
Member

samdark left a comment

Looks like backwards compatibility is fine.

@samdark samdark merged commit 472600e into yiisoft:master Jun 13, 2020
10 of 11 checks passed
10 of 11 checks passed
PHP 5.4 on ubuntu-latest
Details
PHP 5.5 on ubuntu-latest
Details
PHP 5.6 on ubuntu-latest
Details
PHP 7.0 on ubuntu-latest
Details
PHP 7.1 on ubuntu-latest
Details
PHP 7.2 on ubuntu-latest
Details
PHP 7.3 on ubuntu-latest
Details
PHP 7.4 on ubuntu-latest
Details
NPM 6 on ubuntu-latest
Details
codeclimate 5 issues to fix
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@brandonkelly
Copy link
Contributor Author

brandonkelly commented Jun 13, 2020

Thanks @samdark!

@brandonkelly brandonkelly deleted the brandonkelly:primary-replica branch Jun 13, 2020
darkdef added a commit to darkdef/yii2 that referenced this pull request Jun 14, 2020
Fix yiisoft#18102: Use “primary”/“replica” terminology instead of “master”/“…
@lubosdz
Copy link
Contributor

lubosdz commented Jun 14, 2020

I'd never think before that whitelist/blacklist or master/slave in framework regard has anything to do with xenophoby. Every developer knows those are common historical terms used around the IT world for years. This PR does not add any value, and IMHO is too rushy to reflex last 2 week's social events somewhere in America and is a BC break. Framework as a tech stuff should stay away from such a things. Or at least it should be discussed & agreed by a larger Yii community.

@samdark
Copy link
Member

samdark commented Jun 14, 2020

It is not a BC break, only soft deprecation. I've checked it. If it actually causes any issues with existing apps, please report it.

I agree that the PR doesn't add value and may cause a minor inconvenience for people who do not have historical and cultural background similar to US and thus these people, including myself, do not find these terms personally offensive.

I agree that in ideal world technical terms should not offend anyone because the context is different but in reality it's not like that. And that's not only about last 2 weeks but about cultural differences overall.

If new terms would have been any worse than previous ones, I'd likely criticize such pull request but primary/replica terms are alright about understanding and are nowadays widely used as @brandonkelly highlighted.

@rob006
Copy link
Member

rob006 commented Jun 14, 2020

@samdark This PR removes existing and documented properties - of course it breaks BC and it is against documented BC promise. It will break my code if:

  1. I extend one of changed classes and override one of removed properties to change default value (for example set default value for Connection::$slaveConfig) - my property will be ignored after this change.
  2. I extend any of changed classes and implement one of getter/setter methods for different purposes (for example Connection::getSlaveConfig()) - it will either change behavior or throw exception if method signatures does not match.
@brandonkelly
Copy link
Contributor Author

brandonkelly commented Jun 14, 2020

Almost every change could theoretically result in some unintended break. Doesn’t mean it’s a practical concern. And in this case the “break” would be that read queries start pointing to the primary DB server, until you notice that you’re overriding a now-deprecated property. So pretty low risk I’d say.

@rob006
Copy link
Member

rob006 commented Jun 15, 2020

And in this case the “break” would be that read queries start pointing to the primary DB server, until you notice that you’re overriding a now-deprecated property.

That depends on the case, it could bring any effect, from nothing to fatal errors as result of inconsistent configuration. But "read queries start pointing to the primary DB server" is probably the worst of them, because it will be easy to miss it on testing/dev environment and could completely kill your production master server by flood of read queries (previously handled by swarm of read replicas). And finding the cause of this won't be so obvious.

So pretty low risk I’d say.

I'd say that there is not much to gain by this change either, at least from technical perspective. Yii 2 is in maintenance mode, and this is cosmetic change with potential BC break implications. I don't believe that any similar PR (but not politically affected) would be merged, especially so quickly.

BTW: this renaming still could be done and keep BC at the same time by leaving old properties and adding only PC-compliant setters and getters to provide new interface to set primary/replica settings.

@brandonkelly
Copy link
Contributor Author

brandonkelly commented Jun 15, 2020

BTW: this renaming still could be done and keep BC at the same time by leaving old properties and adding only PC-compliant setters and getters to provide new interface to set primary/replica settings.

That’s a great point. Would be a bit simpler to implement (I can remove those docs-only trait classes), and the docs can still point people to the new “properties”. I’ll push that up in a new PR.

@brandonkelly
Copy link
Contributor Author

brandonkelly commented Jun 15, 2020

…and done: #18106

samdark added a commit that referenced this pull request Jun 17, 2020
samdark added a commit that referenced this pull request Jun 17, 2020
…aster”/“slave”"

This reverts commit 472600e.
samdark added a commit that referenced this pull request Jun 17, 2020
samdark added a commit that referenced this pull request Jun 17, 2020
@brandonkelly
Copy link
Contributor Author

brandonkelly commented Jun 17, 2020

This change ended up getting reverted due to backwards compatibility concerns (see #18106).

I’ve made similar changes to Craft CMS via our own DB connection class that extends yii\db\Connection (craftcms/cms@656aea8).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.