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

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

Merged
merged 8 commits into from
Jul 30, 2022

Conversation

tlfeng
Copy link
Collaborator

@tlfeng tlfeng commented Jul 27, 2022

Description

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

  • Deprecate class MasterService and create alternative class ClusterManagerService.
  • Add a unit test to validate the method ClusterService.getMasterService() can still return an object in type MasterService.
  • Rename all the existing references of MasterService to ClusterManagerService, and rename the local variable names.
  • Deprecate public methods ClusterServiceUtils.createMasterService(...) and create alternative methods createClusterManagerService(...)

Note:
The class ClusterManagerService is a subclass of MasterService, the inheritance relationship is opposite from most of the other classes with Master in the name (that covered by issue #1684).
The reason is:
There is a public method that has return value in type MasterService,


And in the code for Performance Analyzer plugin, there is a local variable in type MasterService:
https://github.com/opensearch-project/performance-analyzer/blob/5ee4809ac1cda6517ed871aeb12c6635203e7f1d/src/main/java/org/opensearch/performanceanalyzer/collectors/MasterServiceEventMetrics.java#L219
If making the old class MasterService a subclass of the new class ClusterManagerService, the above usage will be broken.
Reversing the inheritance relationship, I'm able to keep the backwards compatibility of the method getMasterService() while deprecating the class MasterService and encourage using a new class ClusterManagerService.

Issues Resolved

A part of issue #3543 and #3544 (comment)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@tlfeng tlfeng added deprecate v3.0.0 Issues and PRs related to version 3.0.0 backport 2.x Backport to 2.x branch v2.2.0 enhancement Enhancement or improvement to existing feature or request labels Jul 27, 2022
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@tlfeng tlfeng marked this pull request as ready for review July 27, 2022 23:02
@tlfeng tlfeng requested review from a team and reta as code owners July 27, 2022 23:02
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2022

Codecov Report

Merging #4022 (0e03f0e) into main (85bfde1) will decrease coverage by 0.01%.
The diff coverage is 74.36%.

@@             Coverage Diff              @@
##               main    #4022      +/-   ##
============================================
- Coverage     70.65%   70.64%   -0.02%     
- Complexity    56904    56950      +46     
============================================
  Files          4579     4583       +4     
  Lines        273780   273931     +151     
  Branches      40143    40158      +15     
============================================
+ Hits         193445   193511      +66     
- Misses        64119    64195      +76     
- Partials      16216    16225       +9     
Impacted Files Coverage Δ
...ation/TransportClusterAllocationExplainAction.java 52.77% <ø> (ø)
...tion/TransportAddVotingConfigExclusionsAction.java 87.50% <ø> (ø)
...on/TransportClearVotingConfigExclusionsAction.java 86.04% <ø> (-2.33%) ⬇️
...ries/cleanup/TransportCleanupRepositoryAction.java 56.86% <ø> (ø)
...tories/delete/TransportDeleteRepositoryAction.java 100.00% <ø> (ø)
...positories/get/TransportGetRepositoriesAction.java 9.37% <ø> (ø)
...repositories/put/TransportPutRepositoryAction.java 100.00% <ø> (ø)
...tories/verify/TransportVerifyRepositoryAction.java 33.33% <ø> (ø)
...cluster/reroute/TransportClusterRerouteAction.java 44.00% <ø> (+1.33%) ⬆️
...settings/TransportClusterUpdateSettingsAction.java 51.72% <ø> (ø)
... and 604 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

Copy link
Member

@kotwanikunal kotwanikunal left a comment

Choose a reason for hiding this comment

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

LGTM apart from the FakeThreadPoolMasterService comment.

@@ -84,21 +84,21 @@ public void testFakeClusterManagerService() {
doAnswer(invocationOnMock -> runnableTasks.add((Runnable) invocationOnMock.getArguments()[0])).when(executorService).execute(any());
when(mockThreadPool.generic()).thenReturn(executorService);

FakeThreadPoolMasterService masterService = new FakeThreadPoolMasterService(
FakeThreadPoolMasterService clusterManagerService = new FakeThreadPoolMasterService(
Copy link
Member

Choose a reason for hiding this comment

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

Does the class FakeThreadPoolMasterService need to be renamed as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kotwanikunal Thanks for your review. I forgot to write a note for this. 😂
I'm not going to rename this class in this PR. I plan to use the way of dealing with other classes on the 3 classes: FakeThreadPoolMasterService BlockMasterServiceOnMaster BusyMasterServiceDisruption.
One PR to rename directly, and one PR to create a new class with the old name then make it a subclass of the class in new name.

Copy link
Member

Choose a reason for hiding this comment

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

👍 . Hopefully you have a task to keep track of these 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😁 I listed the remaining items in this issue #3543 (comment), and I will check it before wrapping up the work.

@tlfeng
Copy link
Collaborator Author

tlfeng commented Jul 28, 2022

@andrross Thanks for your time on this PR! 👍
I will hold on merging this PR until PR #4006 got merged and backported to avoid conflict.

…vice

Signed-off-by: Tianli Feng <[email protected]>

# Conflicts:
#	server/src/main/java/org/opensearch/cluster/service/ClusterService.java
#	server/src/main/java/org/opensearch/common/util/concurrent/BaseFuture.java
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@tlfeng tlfeng merged commit 740f75d into opensearch-project:main Jul 30, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 30, 2022
…ManagerService' (#4022)

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

- Deprecate class `MasterService` and create alternative class `ClusterManagerService`.
- Add a unit test to validate the method `ClusterService.getMasterService()` can still return an object in type `MasterService`.
- Rename all the existing references of `MasterService` to `ClusterManagerService`, and rename the local variable names.
- Deprecate public methods `ClusterServiceUtils.createMasterService(...)` and create alternative methods `createClusterManagerService(...)`

Note:
The class `ClusterManagerService` is a subclass of `MasterService`, the inheritance relationship is opposite from most of the other classes with `Master` in the name (that covered by issue #1684).
The reason is:
There is a public method that has return value in type `MasterService`,
https://github.com/opensearch-project/OpenSearch/blob/388c80ad94529b1d9aad0a735c4740dce2932a32/server/src/main/java/org/opensearch/cluster/service/ClusterService.java#L221
And in the code for Performance Analyzer plugin, there is a local variable in type `MasterService`:
https://github.com/opensearch-project/performance-analyzer/blob/5ee4809ac1cda6517ed871aeb12c6635203e7f1d/src/main/java/org/opensearch/performanceanalyzer/collectors/MasterServiceEventMetrics.java#L219
If making the old class `MasterService` a subclass of the new class `ClusterManagerService`, the above usage will be broken.
Reversing the inheritance relationship, I'm able to keep the backwards compatibility of the method `getMasterService()` while deprecating the class `MasterService` and encourage using a new class `ClusterManagerService`.

Signed-off-by: Tianli Feng <[email protected]>
(cherry picked from commit 740f75d)
@tlfeng tlfeng deleted the deprecate-masterservice branch July 30, 2022 03:41
tlfeng pushed a commit that referenced this pull request Jul 30, 2022
…ManagerService' (#4022) (#4050)

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

- Deprecate class `MasterService` and create alternative class `ClusterManagerService`.
- Add a unit test to validate the method `ClusterService.getMasterService()` can still return an object in type `MasterService`.
- Rename all the existing references of `MasterService` to `ClusterManagerService`, and rename the local variable names.
- Deprecate public methods `ClusterServiceUtils.createMasterService(...)` and create alternative methods `createClusterManagerService(...)`

Note:
The class `ClusterManagerService` is a subclass of `MasterService`, the inheritance relationship is opposite from most of the other classes with `Master` in the name (that covered by issue #1684).
The reason is:
There is a public method that has return value in type `MasterService`,
https://github.com/opensearch-project/OpenSearch/blob/388c80ad94529b1d9aad0a735c4740dce2932a32/server/src/main/java/org/opensearch/cluster/service/ClusterService.java#L221
And in the code for Performance Analyzer plugin, there is a local variable in type `MasterService`:
https://github.com/opensearch-project/performance-analyzer/blob/5ee4809ac1cda6517ed871aeb12c6635203e7f1d/src/main/java/org/opensearch/performanceanalyzer/collectors/MasterServiceEventMetrics.java#L219
If making the old class `MasterService` a subclass of the new class `ClusterManagerService`, the above usage will be broken.
Reversing the inheritance relationship, I'm able to keep the backwards compatibility of the method `getMasterService()` while deprecating the class `MasterService` and encourage using a new class `ClusterManagerService`.

Signed-off-by: Tianli Feng <[email protected]>
(cherry picked from commit 740f75d)
tlfeng pushed a commit that referenced this pull request Aug 1, 2022
…n directory 'test/framework' (#4051)

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

After the class MasterService has been deprecated and class ClusterManagerService has been created in #4022 / commit 740f75d, the classes in `test/framework` directory with name 'MasterService' can be deprecated and renamed.

- Rename the following classes in `test/framework` directory:

FakeThreadPoolMasterService -> FakeThreadPoolClusterManagerService
BlockMasterServiceOnMaster -> BlockClusterManagerServiceOnClusterManager
BusyMasterServiceDisruption -> BusyClusterManagerServiceDisruption

In the following commit, I will add back the above 3 classes with the old name and deprecate them, to keep the backwards compatibility.

Signed-off-by: Tianli Feng <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 1, 2022
…n directory 'test/framework' (#4051)

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

After the class MasterService has been deprecated and class ClusterManagerService has been created in #4022 / commit 740f75d, the classes in `test/framework` directory with name 'MasterService' can be deprecated and renamed.

- Rename the following classes in `test/framework` directory:

FakeThreadPoolMasterService -> FakeThreadPoolClusterManagerService
BlockMasterServiceOnMaster -> BlockClusterManagerServiceOnClusterManager
BusyMasterServiceDisruption -> BusyClusterManagerServiceDisruption

In the following commit, I will add back the above 3 classes with the old name and deprecate them, to keep the backwards compatibility.

Signed-off-by: Tianli Feng <[email protected]>
(cherry picked from commit bea5d1a)
tlfeng pushed a commit that referenced this pull request Aug 1, 2022
…n directory 'test/framework' (#4051) (#4057)

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

After the class MasterService has been deprecated and class ClusterManagerService has been created in #4022 / commit 740f75d, the classes in `test/framework` directory with name 'MasterService' can be deprecated and renamed.

- Rename the following classes in `test/framework` directory:

FakeThreadPoolMasterService -> FakeThreadPoolClusterManagerService
BlockMasterServiceOnMaster -> BlockClusterManagerServiceOnClusterManager
BusyMasterServiceDisruption -> BusyClusterManagerServiceDisruption

In the following commit, I will add back the above 3 classes with the old name and deprecate them, to keep the backwards compatibility.

Signed-off-by: Tianli Feng <[email protected]>
(cherry picked from commit bea5d1a)

Co-authored-by: Tianli Feng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch deprecate enhancement Enhancement or improvement to existing feature or request v2.2.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants