[2.x] Deprecate public class names with master terminology (#3871) #3914
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
…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 <[email protected]>
tlfeng
added
enhancement
Gradle Check (Jenkins) Run Completed with:
|
dreamer-89
reviewed
Jul 15, 2022
Comment on lines
10
to
16
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at |
There was a problem hiding this comment.
For new files, we don't need this part. This comment applies for all new files containing this licensing term.
There was a problem hiding this comment.
Thanks for your review! Because the file name doesn't change compared to before, so initially I didn't take them as new files. But it is reasonable to remove this part from license header.
There was a problem hiding this comment.
Removed those license header in commit ebb2bc9. 😄
There was a problem hiding this comment.
Instead of changing the backport PR for this change. Let's create a new one and change here
There was a problem hiding this comment.
@owaiskazi19 I will create a new PR to change the license header in main branch. Thanks for your advice!
There was a problem hiding this comment.
Created new PR to remove the part of license header in main branch #3924
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Tianli Feng <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Tianli Feng <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## 2.x #3914 +/- ##
============================================
- Coverage 70.64% 70.62% -0.03%
- Complexity 56398 56429 +31
============================================
Files 4520 4526 +6
Lines 271675 271706 +31
Branches 39974 39974
============================================
- Hits 191932 191890 -42
- Misses 63605 63724 +119
+ Partials 16138 16092 -46
Continue to review full report at Codecov.
|
dreamer-89
approved these changes
Jul 15, 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
Backport PR #3871 / commit 0e96a87
LocalNodeMasterListenerremains.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 was 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.