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

Don't use master/slave terminology (non-breaking changes only) #36

Open
zuiderkwast opened this issue Mar 26, 2024 · 23 comments
Open

Don't use master/slave terminology (non-breaking changes only) #36

zuiderkwast opened this issue Mar 26, 2024 · 23 comments
Labels
de-crapify Correct crap decisions made in the past major-decision-pending Major decision pending by TSC team

Comments

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Mar 26, 2024

Primary and replica are the preferred terms.

The current situation is master/replica, with slave kept as an alias of replica. Some commands like ROLE still returns "master" and "slave".

So far "master" was accepted, so there aren't yet any aliases for "master". Let's introduce aliases and deprecate master.

  1. Add aliases for master (not a breaking change)
  2. Delete master and slave terminology (breaking change) EDIT: After feedback, we've decided to skip the breaking changes. We can only that as a client opt-in. See [NEW] Opt-in for inclusive language (primary/replia in ROLE reply, CLUSTER SHARDS, etc.) #751.

Occurrences of "master" where we can add "primary" and make "master" an alias:

  • CLIENT KILL TYPE MASTER
  • CLIENT LIST TYPE MASTER
  • SENTINEL GET-MASTER-ADDR-BY-NAME

Note: This issue was edited to not include any breaking changes. The initial suggestion included breaking changes, but this was not appreciated by the users. Thus, the downvotes.

@zuiderkwast zuiderkwast mentioned this issue Mar 26, 2024
10 tasks
@GaryPWhite
Copy link

GaryPWhite commented Mar 26, 2024

Hi! Would this be a decent "first issue" for a techie who's interested in getting more involved with the alt/redis ecosystem? I'd be happy to use this as a way to get more familiar with the repo :)

@zuiderkwast
Copy link
Contributor Author

@GaryPWhite Yes, that's great, but first we need to analyze and decide exactly what we need to do and in which steps. There are implications like Redis compatibility to consider.

@zuiderkwast
Copy link
Contributor Author

@GaryPWhite Actually I think you can start with the non-breaking changes of simply adding PRIMARY aliases (or rather make MASTER an alias of PRIMARY) for these:

  • CLIENT KILL TYPE MASTER
  • CLIENT LIST TYPE MASTER
  • SENTINEL GET-MASTER-ADDR-BY-NAME

Look into how SLAVE is already an alias of REPLICA in these commands.

@hwware
Copy link
Member

hwware commented Mar 26, 2024

Agree with master as an alias of primary. But suggest to hold the master and slave name, maybe they will remove in future version

@zuiderkwast
Copy link
Contributor Author

Yes, the breaking change is a "major decision". We should analyze the implications a bit more too. There may be reasons to be compatible with other forks and implementations so we should also keep on eye on them.

We may even consider some possibility for conditional compilation or similar.

@PingXie
Copy link
Member

PingXie commented Mar 26, 2024

I would vote for "primary" and "replica".

As for changes, I now think more and more that we need a "compat" knob. It can be either compile time or runtime (via config).

We could also consider a staged change:

  1. Anything non-user facing, i.e., comments, function/variable names, etc

  2. Aliasing input to support both terminologies (but also under the "compat" switch)

  3. Changing output. Aliasing would not work in this case. We will default to the new terminology but it can be overridden by the user, again either compile time or runtime (pending decision)

@zuiderkwast
Copy link
Contributor Author

Yes, 1 and 2 can be done immediately IMO, but in two separate PRs.

@stockholmux
Copy link
Contributor

If there is a compat knob, it would be good to have a pre-determined sequence of deprecation.

E.g. version n introduce knob and optional new terminology, version n+1 new terminology becomes default, old terminology is deprecated, version n+2 remove old terminology.

Otherwise these things just linger. Pull off the bandaid, as they say.

@PingXie
Copy link
Member

PingXie commented Mar 26, 2024

Pull off the bandaid, as they say

Yes we should do it at some point but I think the timing of it falls in another major decision and we need to give our users ample time to adjust. As much as I am personally in strong favor of ripping this bandaid off asap I also understand that the success of this project is very much dependent on "continuity". It takes time to turn this huge ship around.

@stockholmux
Copy link
Contributor

stockholmux commented Mar 26, 2024

3 major version would be enough transition time, I'd assume.

But mainly I'm just advocating have a set plan before changes happen so it doesn't languish for an undetermined amount of time.

@zuiderkwast zuiderkwast added the de-crapify Correct crap decisions made in the past label Mar 26, 2024
@PingXie
Copy link
Member

PingXie commented Mar 26, 2024

I'm just advocating have a set plan before changes happen so it doesn't languish for an undetermined amount of time.

Timing aside, I think one option, as you suggested too, is to just remove this compat knob and we are done. I have yet to think of other options because as far as output is concerned, it is a binary decision. So the plan in my opinion is more about deliberating a timeline with the help of the community, than an engineering plan to help with a smooth transition. In other words, I think #36 (comment) is the best we can do. Curious to hear other folks' thoughts.

@hpatro
Copy link
Contributor

hpatro commented Mar 26, 2024

de-crapify 😂

@DarrenJiang13
Copy link

Maybe we need to consider some compatibility to SDKs like jedis.

Like "cluster nodes", it may return with "master" field and might be used by some SDKs.

@zuiderkwast
Copy link
Contributor Author

@alonohana627 May I ask why you down-voted this? Just curious. If it's about compatibility, I'd like to hear it so we can make a better decision.

@singlyfy
Copy link

@alonohana627 May I ask why you down-voted this? Just curious. If it's about compatibility, I'd like to hear it so we can make a better decision.

all his (recently) created github repositories are using "master" for branch naming,
so I think it is personal preference in this case

@madolson madolson added the major-decision-pending Major decision pending by TSC team label Mar 31, 2024
@alonohana627
Copy link

alonohana627 commented Apr 1, 2024

@singlyfy @zuiderkwast sry for downvote!!! I don't have an opinion, when I create a repo I don't consider the main's repo name. It was a missclick. I remove the downvote.

I want to contribute but I'm new, I'm not sure if upvote/downvote this suggestion because I sincerely don't understand it. I hope you won't think I tried to troll or anything, sorry for the inconvenient.

@Dom4n
Copy link

Dom4n commented Apr 2, 2024

I have down-voted only because I don't encourage making breaking changes, aliases are totally fine.
But please do not force name change by removing old naming conventions.

@zuiderkwast
Copy link
Contributor Author

@Dom4n It is a very valid concern. I think backward compatibility is very important, especially in this kind of project which has been mostly feature-complete since many years. Let's think about it a bit more.

@zuiderkwast
Copy link
Contributor Author

@GaryPWhite Are you still interesting in helping out with the non-breaking changes?

We will not do any of the breaking changes at this point.

@GaryPWhite
Copy link

absolutely interested -- sorry for the delay. Felt like a lot happened at once -- I'll start taking a look!

@GaryPWhite
Copy link

GaryPWhite commented May 15, 2024

@zuiderkwast , I have taken a look and don't really have time to dig into this and do it right. Other folks should feel free to pick this up :) sorry for whiplast!

@zuiderkwast zuiderkwast changed the title finally remove all master/slave terminology Don't use master/slave terminology (non-breaking changes only) Jul 5, 2024
@robd003
Copy link

robd003 commented Jul 12, 2024

There is zero logical reason to make a breaking name change like this. It's just going to cause issues with 3rd party tools.

Nothing of value will be gained by doing this.

@madolson
Copy link
Member

There is zero logical reason to make a breaking name change like this

We're only making the non-breaking changes. Anything part of the API (the command names) or API responses will remain the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
de-crapify Correct crap decisions made in the past major-decision-pending Major decision pending by TSC team
Projects
Status: Optional for rc1
Development

No branches or pull requests