Deprecate public methods and variables that contain 'master' terminology in class 'NoMasterBlockService' and 'MasterService' by tlfeng · Pull Request #4006 · opensearch-project/OpenSearch

tlfeng · 2022-07-25T21:03:30Z

Description

To support inclusive language, the master terminology is going to be replaced by cluster manager in the code base.

Deprecated public methods and variables in class NoMasterBlockService and MasterService, and created alternative methods or variables.
A unit test is added to validate the change of thread name that identifies the thread running update task for MasterService.

List of public methods and variables that deprecated.
In class NoMasterBlockService:

int NO_MASTER_BLOCK_ID
ClusterBlock NO_MASTER_BLOCK_WRITES
ClusterBlock NO_MASTER_BLOCK_ALL
ClusterBlock NO_MASTER_BLOCK_METADATA_WRITES
ClusterBlock getNoMasterBlock()

In class MasterService:

String MASTER_UPDATE_THREAD_NAME
boolean assertMasterUpdateThread()
boolean assertNotMasterUpdateThread(String reason)

Issues Resolved

A part of issue #3543

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.

…ogy in class NoMasterBlockService and MasterService Signed-off-by: Tianli Feng <ftianli@amazon.com>

github-actions · 2022-07-25T21:28:00Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/969/
CommitID: fae7a08

codecov-commenter · 2022-07-25T21:29:07Z

Codecov Report

❌ Patch coverage is 78.12500% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.34%. Comparing base (efde8c5) to head (a7366c4).
⚠️ Report is 4776 commits behind head on main.

Files with missing lines	Patch %	Lines
.../org/opensearch/cluster/service/MasterService.java	62.50%	2 Missing and 1 partial ⚠️
...g/opensearch/cluster/coordination/Coordinator.java	50.00%	0 Missing and 2 partials ⚠️
...org/opensearch/cluster/service/ClusterService.java	0.00%	1 Missing ⚠️
.../opensearch/common/util/concurrent/BaseFuture.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4006      +/-   ##
============================================
- Coverage     70.51%   70.34%   -0.17%     
+ Complexity    56800    56711      -89     
============================================
  Files          4573     4573              
  Lines        273476   273485       +9     
  Branches      40101    40102       +1     
============================================
- Hits         192844   192387     -457     
- Misses        64420    64870     +450     
- Partials      16212    16228      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Tianli Feng <ftianli@amazon.com>

github-actions · 2022-07-26T06:45:20Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/981/
CommitID: a7366c4

nknize

The PR largely LGTM. I have one question before giving a full approval. I'd like us to be thoughtful about public static modifiers because they will get used / abused by extensions if they are available and not marked as @internal. Do we want these static variables to be used in extensions? I understand we might need to retain public for internal purposes but if we want to guardrail them against extensions we should apply the @opensearch.internal javadoc tag to signal they should not be used outside of core.

nknize · 2022-07-26T22:05:21Z

server/src/main/java/org/opensearch/cluster/coordination/NoClusterManagerBlockService.java


+    /** @deprecated As of 2.2, because supporting inclusive language, replaced by {@link #NO_CLUSTER_MANAGER_BLOCK_ID} */
+    @Deprecated
+    public static final int NO_MASTER_BLOCK_ID = NO_CLUSTER_MANAGER_BLOCK_ID;


Are we holding on to these because there are plugins or extensions that use this static variable outside of core?

@nknize Thank you so much for your review! 👍
I didn't check whether the public static variables are used in plugins or not. My idea was to keep the compatibility in minor versions, such as 2.x, for all methods and variables that will be renamed.
This change is going to be backported into 2.x branch, so I can't break compatibility for any public method or variable.
After doing a search in GitHub, looks like the variable NO_MASTER_BLOCK_ID is only used in this repository.

I can't find usage of "NO_MASTER_BLOCK_" outside of the OpenSearch repo, so I think all of these are fair game for removal.

Even changing the public variable name is a technical breaking change, after backporting this PR to 2.x branch, is it fine to rename here directly instead of marking as deprecated first?

@tlfeng you are taking the conservative approach and, IMHO, this is a good move. The inherited code base is sloppy wrt visibility modifiers and javadocs and this causes issues and confusion with extensions and external plugins. We're trying to fix that on OpenSearch to support our micro-repository project design.

I presume these variables are "internal use only" thus safe to unilaterally refactor w/o compatibility support. However, since they have no markings I think we should treat them as if an extension is using them and follow the deprecation process. I don't believe we have have this formally written out so I'll jot the process down here (you're largely already following this):

Add deprecation annotation

Update javadocs to communicate usage permissions (@opensearch.internal, @opensearch.experimental, or @opensearch.api)

Add deprecation messages (including the logger, if possible or necessary)

Commit deprecation and backport

Open a new PR that removes in main or refactors visibility modifier

Thoughts? /cc @andrross

@nknize That is definitely the conservative approach and probably the right way to go. It's one extra PR with some trivial removals/changes so that's minimal extra work without having to worry about breaking our promise not to introduce breaking changes in a minor version.

So then I think the only change to this PR would be to add @opensearch.internal to the new refactored variables.

nknize

Let's add javadocs w/ @opensearch.internal to the new refactored variables to signal they are internal use only and could change at any time w/o bwc guarantees. I give one suggested example in the review.

nknize · 2022-07-28T15:54:26Z

server/src/main/java/org/opensearch/cluster/coordination/NoClusterManagerBlockService.java


+    /** @deprecated As of 2.2, because supporting inclusive language, replaced by {@link #NO_CLUSTER_MANAGER_BLOCK_ID} */
+    @Deprecated
+    public static final int NO_MASTER_BLOCK_ID = NO_CLUSTER_MANAGER_BLOCK_ID;


So then I think the only change to this PR would be to add @opensearch.internal to the new refactored variables.

nknize · 2022-07-28T15:58:58Z

server/src/main/java/org/opensearch/cluster/coordination/NoClusterManagerBlockService.java

-    public static final int NO_MASTER_BLOCK_ID = 2;
-    public static final ClusterBlock NO_MASTER_BLOCK_WRITES = new ClusterBlock(
-        NO_MASTER_BLOCK_ID,
+    public static final int NO_CLUSTER_MANAGER_BLOCK_ID = 2;


Suggested change

public static final int NO_CLUSTER_MANAGER_BLOCK_ID = 2;

/**

* Ordinal ID used to block when no cluser manager is available

* @opensearch.internal

*/

public static final int NO_CLUSTER_MANAGER_BLOCK_ID = 2;

Of course feel free to update the javadoc to something more clear.

@nknize Got it. Thanks a lot for your detailed explanation! In yesterday I didn't realize that you would like me to add @opensearch.internal annotation on the variable.
I've got a question, since the whole class has been marked with @opensearch.internal:

OpenSearch/server/src/main/java/org/opensearch/cluster/coordination/NoClusterManagerBlockService.java

Line 47 in 00db112

* @opensearch.internal

)
Is that necessary to mark the variable? or you just want to emphasize it?

Ooh! Good point! No I think that covers it. Thx for pointing it out.

@nknize No problem! Thank you for your suggestion on the action we should make to avoid the public API abuse, this will be a good example. 😄👍

nknize

LGTM! Thx for iterating on this!

…ogy in class 'NoMasterBlockService' and 'MasterService' (#4006) Deprecates public methods and variables that contain 'master' terminology in class NoMasterBlockService and MasterService. Unit tests are also added to ensure consistency with the new naming. Signed-off-by: Tianli Feng <ftianli@amazon.com> (cherry picked from commit e65aaca)

…ogy in class 'NoMasterBlockService' and 'MasterService' (#4006) (#4038) To support inclusive language, the `master` terminology is going to be replaced by `cluster manager` in the code base. - Deprecated public methods and variables in class `NoMasterBlockService` and `MasterService`, and created alternative methods or variables. - A unit test is added to validate the change of thread name that identifies the thread running update task for MasterService. List of public methods and variables that deprecated. In class `NoMasterBlockService`: ``` int NO_MASTER_BLOCK_ID ClusterBlock NO_MASTER_BLOCK_WRITES ClusterBlock NO_MASTER_BLOCK_ALL ClusterBlock NO_MASTER_BLOCK_METADATA_WRITES ClusterBlock getNoMasterBlock() ``` In class `MasterService`: ``` String MASTER_UPDATE_THREAD_NAME boolean assertMasterUpdateThread() boolean assertNotMasterUpdateThread(String reason) ``` Signed-off-by: Tianli Feng <ftianli@amazon.com> (cherry picked from commit e65aaca)

Deprecate public methods and variables that contain 'master' terminol…

fae7a08

…ogy in class NoMasterBlockService and MasterService Signed-off-by: Tianli Feng <ftianli@amazon.com>

tlfeng added enhancement Enhancement or improvement to existing feature or request v3.0.0 Issues and PRs related to version 3.0.0 backport 2.x Backport to 2.x branch v2.2.0 deprecate labels Jul 25, 2022

Deprecate MasterService.MASTER_UPDATE_THREAD_NAME and add unit test

a7366c4

Signed-off-by: Tianli Feng <ftianli@amazon.com>

tlfeng marked this pull request as ready for review July 26, 2022 17:18

tlfeng requested review from a team and reta as code owners July 26, 2022 17:18

tlfeng mentioned this pull request Jul 26, 2022

Deprecate public or protected class that contains "master" terminology in 'server' and 'test/framework' directory #3543

Closed

nknize reviewed Jul 26, 2022

View reviewed changes

andrross approved these changes Jul 27, 2022

View reviewed changes

nknize suggested changes Jul 28, 2022

View reviewed changes

tlfeng mentioned this pull request Jul 28, 2022

Deprecate class 'MasterService' and create alternative class 'ClusterManagerService' #4022

Merged

5 tasks

nknize approved these changes Jul 28, 2022

View reviewed changes

nknize merged commit e65aaca into opensearch-project:main Jul 28, 2022

opensearch-trigger-bot bot mentioned this pull request Jul 28, 2022

[Backport 2.x] Deprecate public methods and variables that contain 'master' terminology in class 'NoMasterBlockService' and 'MasterService' #4038

Merged

tlfeng deleted the deprecate-method-in-some-class-master-block branch July 30, 2022 03:42

Conversation

tlfeng commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

github-actions bot commented Jul 25, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov-commenter commented Jul 25, 2022 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jul 26, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nknize Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tlfeng commented Jul 25, 2022 •

edited

Loading

codecov-commenter commented Jul 25, 2022 •

edited by codecov bot

Loading

nknize Jul 27, 2022 •

edited

Loading