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.

Sign up for GitHub

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

Jump to bottom

Change the "Master" nomenclature #564

Closed
wants to merge 0 commits into from
Closed

Change the "Master" nomenclature #564

wants to merge 0 commits into from

Conversation

sharp-pixel
Copy link
Contributor

Description

Replace master by leader in the context of master node and by main in the context of master git branch when applicable.

Warning: Causes breaking changes in the API _cat/master => _cat/leader and cluster configuration parameters

Needs discussion and validation for these breaking changes.

Issues Resolved

Solves #472

Check List

  • [ x ] New functionality includes testing.
    • [ x ] All tests pass
  • [ x ] New functionality has been documented.
    • [ x ] New functionality has javadoc added
  • [ x ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@odfe-release-bot
Copy link

✅   Gradle Wrapper Validation success c3c0e3b6ca0062ac78571f4907e75dc1b1648bf5

@odfe-release-bot
Copy link

✅   DCO Check Passed c3c0e3b6ca0062ac78571f4907e75dc1b1648bf5

@odfe-release-bot
Copy link

✅   Gradle Precommit success c3c0e3b6ca0062ac78571f4907e75dc1b1648bf5

@peterzhuamazon peterzhuamazon added the community Issues raised by community members and contributors label Apr 15, 2021
@stockholmux
Copy link
Member

I am 100% behind this PR. I know this issue was discussed prior to being made public - I thought there was even an associated issue although I can't find it.

@odfe-release-bot
Copy link

✅   Gradle Wrapper Validation success 874658afc303650e63c8073d5bacb05e3032f493

@odfe-release-bot
Copy link

✅   DCO Check Passed 874658afc303650e63c8073d5bacb05e3032f493

@odfe-release-bot
Copy link

✅   Gradle Precommit success 874658afc303650e63c8073d5bacb05e3032f493

@odfe-release-bot
Copy link

✅   Gradle Wrapper Validation success 10c12129df8f1fd2bae68ccde8e708f5829462db

@odfe-release-bot
Copy link

❌   DCO Check Failed 10c12129df8f1fd2bae68ccde8e708f5829462db
Run ./dev-tools/signoff-check.sh remotes/origin/main 10c12129df8f1fd2bae68ccde8e708f5829462db to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

@odfe-release-bot
Copy link

✅   Gradle Precommit success 10c12129df8f1fd2bae68ccde8e708f5829462db

@dansimpson
Copy link

The only concern I have with this change is that is breaks some APIs. In a perfect world, this would be a deprecation warning first, and then at a later date, after all the libraries are caught up, the change is made.

If 1.0.0 has API compatibility with Elasticsearch, it lowers the barrier of adoption for folks coming from Elasticsearch and Elasticsearch libraries. These APIs are probably not involved in many of the customers hot paths, but Elasticsearch was really good about backwards compatibility, deprecation, and I think that improves the developer experience.

Maybe there is a way to rewrite the parameter names from old to new, with deprecation warnings, and for the rest handlers, perhaps adding support for both paths, with the intent of removing in a future version.

@odfe-release-bot
Copy link

✅   DCO Check Passed ec4658ab5f2db217a1a9abff9f2f5fd124dde7ec

@odfe-release-bot
Copy link

✅   Gradle Wrapper Validation success ec4658ab5f2db217a1a9abff9f2f5fd124dde7ec

@odfe-release-bot
Copy link

✅   Gradle Precommit success ec4658ab5f2db217a1a9abff9f2f5fd124dde7ec

@dblock
Copy link
Member

dblock commented Apr 16, 2021

+1 on the spirit of this PR, but I do think we should preserve backward compatibility, what's involved in doing that here?

@odfe-release-bot
Copy link

✅   Gradle Wrapper Validation success a8278a716da923fc5f16edee23c19fcb12e82d05

@odfe-release-bot
Copy link

✅   DCO Check Passed a8278a716da923fc5f16edee23c19fcb12e82d05

@odfe-release-bot
Copy link

✅   Gradle Precommit success a8278a716da923fc5f16edee23c19fcb12e82d05

@stockholmux
Copy link
Member

I think a middle ground might be to have two user facing APIs: one with new terminology and the other as a deprecated terminology similar to what @dansimpson is saying. By, say, 2.0 the 'm' word is gone fully from APIs.

We document the new terminology so anyone starting off with OpenSearch never starts using the 'm' word.

Mark internal facing uses of the 'm' as a future change and no new code should contain the old terminology. Attack internal changes as the code modified.

(These are my views as a white dude in Canada - other voices are welcome to disagree and tell me I'm flat out wrong)

@sharp-pixel
Copy link
Contributor Author

I agree, breaking things directly is not a good idea, Java class and variable renaming is not a problem, but we should handle both configuration options and APIs.
What's involved is duplicating the API endpoints and adding more tests to check both terminologies, and adding some warning for deprecation in the server logs when a deprecated configuration parameter is used.
So you see anything else?

@morsik
Copy link

morsik commented Apr 18, 2021 •
edited
Loading

Apart from my personal opinions about this very stupid policically-correct change which I already wrote here - leader is totally wrong here. ES master is not "leader" of anything. Master is meant do to some job, not lead other nodes to do something.

The master node is responsible for lightweight cluster-wide actions such as creating or deleting an index, tracking which nodes are part of the cluster, and deciding which shards to allocate to which nodes. It is important for cluster health to have a stable master node.

Please read documentation first (and context that it's used in!) before even trying to do such stupid changes. Changing words because of political corectness is stupid and wrong. But changing words without even thinking what they mean in specific context to ones that doesn't make any sense is even worst and makes confusion.

@stockholmux
Copy link
Member

@sharp-pixel The configuration options are a good point. I imagine the will need to be a startup deprecation notice. My understanding that providing an alternate route for external facing APIs is not a heavy lift.

@CEHENKLE CEHENKLE added the v2.0.0 Version 2.0.0 label May 13, 2021
@nknize nknize added >breaking Identifies a breaking change. discuss Issues intended to help drive brainstorming and decision making stalled Issues that have stalled labels Jun 25, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success a8278a716da923fc5f16edee23c19fcb12e82d05

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed a8278a716da923fc5f16edee23c19fcb12e82d05

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success a8278a716da923fc5f16edee23c19fcb12e82d05

@dblock
Copy link
Member

dblock commented Jul 6, 2021

stupid -> unnecessary or incorrect or something else

I am sympathetic to @morsik saying that leader/follower may not be the right nomenclature for OpenSearch; do we have any thoughts on primary and secondary?

@peternied
Copy link
Member

To me the existing nomenclature and technical utility have the following characteristics:

  1. Authoritative decision maker
  2. Allocation of data storage
  3. System meta-state storage

I am sympathetic to @morsik saying that leader/follower may not be the right nomenclature for OpenSearch; do we have any thoughts on primary and secondary?

Primary comes close (1, 2) - but I have trouble separating its ties to systems where the data storage is on the primary. It can mislead the underlying architecture since all data is on the nodes.

  • Leader, Governor, Delegator, Controller, Manager - All function highly on (1, 2)
  • Allocator, Assigner, Router - Good performance around (2), with some (3)

The problem I run into with all of these terms is that they are often used so they become suffixes to the name, {Domain}{Suffix} e.g. "HttpRequestRouter", "ThreadDelegator", "ScenarioTestLeader"

ClusterManager lands the best for me on all (1,2,3). I think the term works for single node clusters, as a bonus.

@dblock
Copy link
Member

dblock commented Jul 6, 2021

Primary comes close (1, 2) - but I have trouble separating its ties to systems where the data storage is on the primary. It can mislead the underlying architecture since all data is on the nodes.

That's a very good point.

@muralikpbhat
Copy link

Technically, there is a leader election (so it is definitely 'leading' the leader eligible nodes) and it may be ok to call it leader. However, I agree the functionality of master is not well represented in that and also if we have different approach in future to decide the leader rather than election, the name leader will become inappropriate. Internally, java classes and docs around master responsibilities are named as 'cluster coordination'. So it might be best to call it 'ClusterCoordinator'? I would have preferred it to be one word, but calling it just 'Coordinator' is conflicting with coordinator nodes which is responsible for fanout.

@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success a8278a716da923fc5f16edee23c19fcb12e82d05

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed a8278a716da923fc5f16edee23c19fcb12e82d05

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success a8278a716da923fc5f16edee23c19fcb12e82d05

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success a8278a716da923fc5f16edee23c19fcb12e82d05

This was referenced Nov 3, 2021
Closed
Closed
@tlfeng
Copy link
Collaborator

tlfeng commented Nov 23, 2021

Hi @sharp-pixel, we have got a plan to replace the non-inclusive term "master" (#472 (comment)), could you please take a look at the plan and modify your PR?
To sum up,

  1. Could you use clustermanager to replace master? We think it describes the responsibility for the "master" node.
  2. We would like to resolve the issue Replace "master" terminology in code comments, and internal variable, method and class names #1548 as the first step, so please split your PR to have one that only replacing the "master" where the backwards compatibility won't be impacted, and those without touching APIs or settings.
  3. Then resolving the issue Add alternative usages for the REST APIs and configurations that contain "master" terminology #1549 as the next step to make duplicate APIs to keep the backwards compatibility.

Look forward to hearing from you soon. Thank you! 👍

@sharp-pixel
Copy link
Contributor Author

sharp-pixel commented Nov 23, 2021 via email

@sharp-pixel sharp-pixel closed this Dec 7, 2021
ritty27 pushed a commit to ritty27/OpenSearch that referenced this pull request May 12, 2024
@VachaShah @dblock
Post 2.6.0 release update for changelog and compatibility (opensearch…
ddff6ce 
…-project#564)

Signed-off-by: Vacha Shah <vachshah@amazon.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
>breaking Identifies a breaking change. community Issues raised by community members and contributors discuss Issues intended to help drive brainstorming and decision making stalled Issues that have stalled v2.0.0 Version 2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.