-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Replace internal usages of 'master' term in 'server/src/main' directory #2519
Replace internal usages of 'master' term in 'server/src/main' directory #2519
Conversation
…rectory Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
In log 3550:
It's reported in issue #2472 |
In log 3556:
I couldn't reproduce it locally. |
In log 3559:
It's reported in issue #1746. |
In log 3561:
It's reported in issue #1703. |
In Log 3563:
I couldn't reproduce locally. Created issue 2522 #2522 |
start gradle check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR is "Replace internal usage of master
..." do you also want to (shift+F6) refactor ClusterHealthResponse.java#L279 from hasDiscoveredMaster()
to hasDiscoveredManager()
(or some method name consistent w/ everything else)? Or are you worried that will unknowingly break a plugin that might use it (since the method is public)?
Hi @nknize, thanks a lot for reviewing my PR! Yeah, that's my concern, and I wouldn't rename the public methods that published to Maven as Java API in version 2.x, in case the change breaks too many plugins or clients. |
} | ||
|
||
private static String getMasterNodeId(ClusterState clusterState) { | ||
private static String getClusterManagerNodeId(ClusterState clusterState) { | ||
if (clusterState == null) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the getMasterNodeId
below in line 130 need to be changed to getClusterManagerNodeId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @xuezhou25 , thanks a lot for your time in reviewing my PR! 👍👍.
the method getMasterNodeId()
in line 130 is published to Maven as Java API. Please see my comment above: #2519 (comment).
Although the normal way to rename the Java API is to deprecate old method and create new alternative method, there are too many public methods contain name "master", and it will introduce too many extra codes to deprecate them all. So I plan to rename them all in a future major version, and give the other plugins and clients enough notice before renaming.
@@ -201,10 +201,10 @@ protected void doStart(ClusterState clusterState) { | |||
logger.debug("no known master node, scheduling a retry"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Thank you, you are so carefully. Those usages are all missed... 😅
I will change them.
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
…server-main Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2519-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 79eb3b04922f4d4688372b93550ded9feffd7338
# Push it to GitHub
git push --set-upstream origin backport/backport-2519-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x Then, create a pull request where the |
…ry (opensearch-project#2519) Signed-off-by: Tianli Feng <[email protected]>
…ry (#2519) (#3165) Signed-off-by: Tianli Feng <[email protected]>
Description
server/src/main
directory.Replacement rules:
master
->cluster_manager
orclusterManager
(variable name) orcluster-manager
(code comment)Master
->ClusterManager
Issues Resolved
A part of #1548
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.