Deprecate public class names with master terminology #3871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Tianli Feng <ftianli@amazon.com>
tlfeng
added
enhancement
Gradle Check (Jenkins) Run Completed with:
|
|
In build 479: |
added 2 commits
July 12, 2022 16:39
Signed-off-by: Tianli Feng <ftianli@amazon.com>
Signed-off-by: Tianli Feng <ftianli@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
|
In build 486: |
…ist in ExceptionSerializationTests Signed-off-by: Tianli Feng <ftianli@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #3871 +/- ##
============================================
- Coverage 71.10% 70.47% -0.63%
+ Complexity 57060 56663 -397
============================================
Files 4557 4567 +10
Lines 272681 272709 +28
Branches 40038 40038
============================================
- Hits 193884 192193 -1691
- Misses 62773 64299 +1526
- Partials 16024 16217 +193
Continue to review full report at Codecov.
|
Gradle Check (Jenkins) Run Completed with:
|
|
Note: The above build 483 didn't run the test for the latest commit, so it can be ignored. |
added 2 commits
July 13, 2022 15:06
…erListener Signed-off-by: Tianli Feng <ftianli@amazon.com>
…eprecated master setting Signed-off-by: Tianli Feng <ftianli@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
|
In build 559: The below is reported in issue #3872 |
reta
approved these changes
Jul 13, 2022
|
Hi @reta, thanks a lot for your review! It really helps me push the process forward！ |
andrross
reviewed
Jul 13, 2022
Comment on lines
44
to
47
| // TODO: add final keyword to the class and private keyword to the default constructor, | ||
| // after removing the deprecated class MasterNodeChangePredicate. | ||
| // Removed the final keyword temporarily only for making the class MasterNodeChangePredicate as a subclass, | ||
| // so that preserving the both class names by maintaining one class implementation for backwards compatibility. |
There was a problem hiding this comment.
I believe this class only exists as a container for a single static method. Assuming that's right, the less intrusive approach may be just to do the following in the other class (as opposed to using inheritance):
@Deprecated
public final class MasterNodeChangePredicate {
private MasterNodeChangePredicate() { }
public static Predicate<ClusterState> build(ClusterState currentState) {
return ClusterManagerNodeChangePredicate.build(clusterState);
}
}Because it means you don't have to change this class at all.
There was a problem hiding this comment.
Thank you so much for providing the suggestion to me! Let me check this approach. 😄
There was a problem hiding this comment.
This must be the "delegation" approach. Thanks for showing me the code. I updated in the commit d10fec2
…sterService to keep backwards compatibility Signed-off-by: Tianli Feng <ftianli@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
tlfeng
commented
Jul 14, 2022
| public MasterService(Settings settings, ClusterSettings clusterSettings, ThreadPool threadPool) { | ||
| super(settings, clusterSettings, threadPool); | ||
| } | ||
|
|
There was a problem hiding this comment.
There is one problem, the return value of getMasterService() should be the old MasterService class, otherwise there will be down-casting problem for the existing classes that uses the return value.
https://github.com/tlfeng/OpenSearch/blob/951af92cb316bf0d9f1b56338fd0bec90d029375/server/src/main/java/org/opensearch/cluster/service/ClusterService.java#L221
public ClusterManagerService getMasterService() {
return clusterManagerService;
}
There was a problem hiding this comment.
Finally I reverted renaming the class MasterService in this PR (in commit 50a322b).
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Tianli Feng <ftianli@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
This reverts commit 694d876. Signed-off-by: Tianli Feng <ftianli@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
added 3 commits
July 14, 2022 13:52
Signed-off-by: Tianli Feng <ftianli@amazon.com>
Signed-off-by: Tianli Feng <ftianli@amazon.com>
…eDisruption Signed-off-by: Tianli Feng <ftianli@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Tianli Feng <ftianli@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
tlfeng
pushed a commit
that referenced
this pull request
Jul 14, 2022
#3870) * Rename public classes with 'Master' to 'ClusterManager' (#3619) Replace master terminology by cluster manager in the public Java APIs to support inclusive language. The PR deal with the public class name in the repository (except for those covered in issue #3542). * Replace `Master` to `ClusterManager` in most of the class names (except `MasterService` class), including all the references to the classes. The next PR will be #3871, adding back the classes in old name for backwards compatibility. List of classes that renamed in this PR: `sever` directory: interface LocalNodeMasterListener -> LocalNodeClusterManagerListener final class MasterNodeChangePredicate -> ClusterManagerNodeChangePredicate NotMasterException -> NotClusterManagerException NoMasterBlockService -> NoClusterManagerBlockService UnsafeBootstrapMasterCommand - UnsafeBootstrapClusterManagerCommand MasterNotDiscoveredException -> ClusterManagerNotDiscoveredException RestMasterAction -> RestClusterManagerAction List of classes that not renamed: `sever` directory: MasterService `test/framework` directory: FakeThreadPoolMasterService BlockMasterServiceOnMaster BusyMasterServiceDisruption Signed-off-by: Tianli Feng <ftianli@amazon.com> (cherry picked from commit a7e113a)
|
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-3871-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0e96a878a5ab1ffec3ee04f15fe976d34c9905dc
# Push it to GitHub
git push --set-upstream origin backport/backport-3871-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.xThen, create a pull request where the |
tlfeng
pushed a commit
to tlfeng/OpenSearch
that referenced
this pull request
Jul 15, 2022
…ect#3871) - Add back the usage of old names for some classes for backwards compatibility. Those public classes were renamed from "master" to "cluster manager" In PR opensearch-project#3619 / commit opensearch-project@a7e113a. - Add a unit test to validate the backwards compatibility of interface `LocalNodeMasterListener` remains. - Revert the renaming of class `MasterService`, `FakeThreadPoolMasterService`, `BlockMasterServiceOnMaster` and `BusyMasterServiceDisruption`, as well as instance variable names of class `MasterService`. Because I couldn't solve the compatibility problem of a public method `public ClusterManagerService getMasterService()` (opensearch-project#3871 (comment)) **List of classes that renamed in previous PR:** In this PR, the usages of the old names should all be restored. `sever` directory: interface LocalNodeMasterListener -> LocalNodeClusterManagerListener final class MasterNodeChangePredicate -> ClusterManagerNodeChangePredicate NotMasterException -> NotClusterManagerException NoMasterBlockService -> NoClusterManagerBlockService UnsafeBootstrapMasterCommand - UnsafeBootstrapClusterManagerCommand MasterNotDiscoveredException -> ClusterManagerNotDiscoveredException RestMasterAction -> RestClusterManagerAction **List of classes that not renamed:** `sever` directory: MasterService `test/framework` directory: FakeThreadPoolMasterService BlockMasterServiceOnMaster BusyMasterServiceDisruption Signed-off-by: Tianli Feng <ftianli@amazon.com>
5 tasks
tlfeng
pushed a commit
that referenced
this pull request
Jul 15, 2022
…3914) * Deprecate public class names with master terminology (#3871) - Add back the usage of old names for some classes for backwards compatibility. Those public classes were renamed from "master" to "cluster manager" In PR #3619 / commit a7e113a. - Add a unit test to validate the backwards compatibility of interface `LocalNodeMasterListener` remains. - Revert the renaming of class `MasterService`, `FakeThreadPoolMasterService`, `BlockMasterServiceOnMaster` and `BusyMasterServiceDisruption`, as well as instance variable names of class `MasterService`. Because I couldn't solve the compatibility problem of a public method `public ClusterManagerService getMasterService()` (#3871 (comment)) **List of classes that renamed in previous PR:** In this PR, the usages of the old names should all be restored. `sever` directory: interface LocalNodeMasterListener -> LocalNodeClusterManagerListener final class MasterNodeChangePredicate -> ClusterManagerNodeChangePredicate NotMasterException -> NotClusterManagerException NoMasterBlockService -> NoClusterManagerBlockService UnsafeBootstrapMasterCommand - UnsafeBootstrapClusterManagerCommand MasterNotDiscoveredException -> ClusterManagerNotDiscoveredException RestMasterAction -> RestClusterManagerAction **List of classes that not renamed:** `sever` directory: MasterService `test/framework` directory: FakeThreadPoolMasterService BlockMasterServiceOnMaster BusyMasterServiceDisruption Signed-off-by: Tianli Feng <ftianli@amazon.com>
This was referenced Jul 19, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
A step of replacing
masterterminology bycluster managerin the public Java APIs to support inclusive language.LocalNodeMasterListenerremains.MasterService,FakeThreadPoolMasterService,BlockMasterServiceOnMasterandBusyMasterServiceDisruption, as well as instance variable names of classMasterService.Because I couldn't solve the compatibility problem of a public method
public ClusterManagerService getMasterService()(Deprecate public class names with master terminology #3871 (comment))Note:
LocalNodeClusterManagerListener(new name) /LocalNodeMasterListener(old name) is used as the return value for the public methodnewHashPublisher(). SinceLocalNodeClusterManagerListeneris the superclass ofLocalNodeMasterListener, changing the return value to a superclass will be a breaking change, it will lead to down-casting for others who assigning the return value to a variable, so I changed the return value back to the old class nameLocalNodeMasterListener.ExceptionSerializationTestsrequires all subclass of OpenSearchException must be registered in the Exception list (code: https://github.com/opensearch-project/OpenSearch/blob/2.1.0/server/src/test/java/org/opensearch/ExceptionSerializationTests.java#L237). I modified the test to ignore the deprecated exception classesNotMasterExceptionandMasterNotDiscoveredException, since there are new classesNotClusterManagerExceptionandClusterManagerNotDiscoveredExceptionas the replacement.List of classes that renamed in previous PR:
In this PR, the usages of the old names should all be restored.
severdirectory:interface LocalNodeMasterListener -> LocalNodeClusterManagerListener
final class MasterNodeChangePredicate -> ClusterManagerNodeChangePredicate
NotMasterException -> NotClusterManagerException
NoMasterBlockService -> NoClusterManagerBlockService
UnsafeBootstrapMasterCommand - UnsafeBootstrapClusterManagerCommand
MasterNotDiscoveredException -> ClusterManagerNotDiscoveredException
RestMasterAction -> RestClusterManagerAction
List of classes that not renamed:
severdirectory:MasterService
test/frameworkdirectory:FakeThreadPoolMasterService
BlockMasterServiceOnMaster
BusyMasterServiceDisruption
Issues Resolved
The second step to resolve issue #3543
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.