Change the "Master" nomenclature #564
Conversation
|
✅ Gradle Wrapper Validation success c3c0e3b6ca0062ac78571f4907e75dc1b1648bf5 |
|
✅ DCO Check Passed c3c0e3b6ca0062ac78571f4907e75dc1b1648bf5 |
|
✅ Gradle Precommit success c3c0e3b6ca0062ac78571f4907e75dc1b1648bf5 |
peterzhuamazon
added
the
community
|
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. |
|
✅ Gradle Wrapper Validation success 874658afc303650e63c8073d5bacb05e3032f493 |
|
✅ DCO Check Passed 874658afc303650e63c8073d5bacb05e3032f493 |
|
✅ Gradle Precommit success 874658afc303650e63c8073d5bacb05e3032f493 |
|
✅ Gradle Wrapper Validation success 10c12129df8f1fd2bae68ccde8e708f5829462db |
|
❌ DCO Check Failed 10c12129df8f1fd2bae68ccde8e708f5829462db |
|
✅ Gradle Precommit success 10c12129df8f1fd2bae68ccde8e708f5829462db |
|
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. |
|
✅ DCO Check Passed ec4658ab5f2db217a1a9abff9f2f5fd124dde7ec |
|
✅ Gradle Wrapper Validation success ec4658ab5f2db217a1a9abff9f2f5fd124dde7ec |
|
✅ Gradle Precommit success ec4658ab5f2db217a1a9abff9f2f5fd124dde7ec |
|
+1 on the spirit of this PR, but I do think we should preserve backward compatibility, what's involved in doing that here? |
|
✅ Gradle Wrapper Validation success a8278a716da923fc5f16edee23c19fcb12e82d05 |
|
✅ DCO Check Passed a8278a716da923fc5f16edee23c19fcb12e82d05 |
|
✅ Gradle Precommit success a8278a716da923fc5f16edee23c19fcb12e82d05 |
|
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) |
|
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. |
|
Apart from my personal opinions about this very stupid policically-correct change which I already wrote here -
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. |
|
@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. |
nknize
added
>breaking
|
✅ Gradle Wrapper Validation success a8278a716da923fc5f16edee23c19fcb12e82d05 |
|
✅ DCO Check Passed a8278a716da923fc5f16edee23c19fcb12e82d05 |
|
✅ Gradle Precommit success a8278a716da923fc5f16edee23c19fcb12e82d05 |
|
I am sympathetic to @morsik saying that leader/follower may not be the right nomenclature for OpenSearch; do we have any thoughts on |
|
To me the existing nomenclature and technical utility have the following characteristics:
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.
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"
|
That's a very good point. |
|
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. |
|
Can one of the admins verify this patch? |
|
✅ Gradle Wrapper Validation success a8278a716da923fc5f16edee23c19fcb12e82d05 |
|
✅ DCO Check Passed a8278a716da923fc5f16edee23c19fcb12e82d05 |
|
✅ Gradle Precommit success a8278a716da923fc5f16edee23c19fcb12e82d05 |
|
✅ Gradle Wrapper Validation success a8278a716da923fc5f16edee23c19fcb12e82d05 |
|
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?
Look forward to hearing from you soon. Thank you! 👍 |
|
Hi, I will start working on proposed changes. Thanks for constructive
input!
…On Tue, Nov 23, 2021 at 7:50 PM Tianli Feng ***@***.***> wrote:
Hi @sharp-pixel <https://github.com/sharp-pixel>, we have got a plan to
replace the non-inclusive term "master" (#472 (comment)
<#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 #1548
<#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 #1549
<#1549> as the
next step to make duplicate APIs to keep the backwards compatibility.
Look forward to hearing from you soon. Thank you! 👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#564 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACDXIE3L3AHFUMIID3ZHRXTUNPPARANCNFSM4275X4CA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
…-project#564) Signed-off-by: Vacha Shah <[email protected]> Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]> Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
Description
Replace
masterbyleaderin the context of master node and bymainin the context of master git branch when applicable.Warning: Causes breaking changes in the API
_cat/master=>_cat/leaderand cluster configuration parametersNeeds discussion and validation for these breaking changes.
Issues Resolved
Solves #472
Check List
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.