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

[2.0] Replace 'master' terminology with 'cluster manager' in log messages in 'server/src/main' directory - Part 2 #3174

Merged
merged 2 commits into from
May 6, 2022

Conversation

tlfeng
Copy link
Collaborator

@tlfeng tlfeng commented May 3, 2022

Description

In PR #2575, most of the master terminology in log messages is replaced by cluster-manager, but there are a few remaining.
This PR replaces more master word with cluster-manager in server/src/main directory that used in log messages.

Issues Resolved

#2539

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • 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.

@tlfeng tlfeng added the v2.0.0 Version 2.0.0 label May 3, 2022
@tlfeng tlfeng requested a review from a team as a code owner May 3, 2022 22:28
@tlfeng tlfeng changed the title Replace 'master' terminology with 'cluster manager' in log and other messages - Part 2 Replace 'master' terminology with 'cluster manager' in log messages in server directory - Part 2 May 3, 2022
@tlfeng tlfeng changed the title Replace 'master' terminology with 'cluster manager' in log messages in server directory - Part 2 [2.0] Replace 'master' terminology with 'cluster manager' in log messages in server directory - Part 2 May 3, 2022
@@ -208,7 +208,7 @@ public void testTwoNodesNoMasterBlock() throws Exception {
otherNode = node1Name.equals(masterNode) ? node2Name : node1Name;
logger.info("--> add voting config exclusion for master node, to be sure it's not elected");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still a few master references on L#209, L#158, L#173

Copy link
Collaborator Author

@tlfeng tlfeng May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your careful review!
Ah, I didn't realized there are still lots of log messages contain master in server/src/internalClusterTest directory and is shown in this PR. 😂
It definitely doesn't look good. Thanks for pointing out!
To minimize the change for 2.0 version, I propose to limit the scope of this PR to server/src/main directory. For the master references in other directories, I will create PR to main branch first and then backport to 2.x branch.
So I will revert those changes in server/src/internalClusterTest directory. Is that fine?

Copy link
Collaborator Author

@tlfeng tlfeng May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes for this PR is actually a subset of #2519. I didn't plan to backport that PR to 2.0 branch since the feature for 2.0 version is frozen and I want to reduce the code change, but when I saw there are still log messages contain master terminology, I think it's worth to replace all master reference in log message in 2.0 version for consistency.
I will make sure all the log message in server/src/main don't have master terminology, since it will be exposed to the user.
For the master terminology in other directories, I will handle in different PRs ( tracked in issue #1548)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 😄

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 9925286
Log 4973

Reports 4973

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 52d4b62
Log 4975

Reports 4975

@tlfeng tlfeng changed the title [2.0] Replace 'master' terminology with 'cluster manager' in log messages in server directory - Part 2 [2.0] Replace 'master' terminology with 'cluster manager' in log messages in 'server/src/main' directory - Part 2 May 4, 2022
@dblock dblock requested a review from kotwanikunal May 4, 2022 18:00
@dblock dblock merged commit bab1fdf into opensearch-project:2.0 May 6, 2022
@tlfeng tlfeng deleted the 2.0-log-replace-master branch May 9, 2022 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.0.0 Version 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants