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 public or protected class that contains "master" terminology in 'server' and 'test/framework' directory #3543

Closed
tlfeng opened this issue Jun 8, 2022 · 3 comments
Labels
deprecate enhancement Enhancement or improvement to existing feature or request v2.2.0

Comments

@tlfeng
Copy link
Collaborator

tlfeng commented Jun 8, 2022

Is your feature request related to a problem? Please describe.
A part of issue #1684.
To support inclusive language, the master terminology is going to be replaced by cluster manager in the code base.

The goal for this issue is:

  1. Deprecate the public/protected classes that has "master" in the names. The involved classes are located in server and test/framework directories, and the deprecated classes will be removed in the next major version (3.0)
  2. Have alternative class aside, which replaced the non-inclusive name "master" and have the same functionality with the deprecated class, and preserve the git history of the deprecated class as the successor.
  3. Rename all public/protected methods or variables contain "master" in the old class, and have alternative new method or variable name in the new class.

Describe the solution you'd like
For overall solution to replace "master" in public Java APIs: #1684 (comment)

Key points:

  1. At the time of deprecating the package, all public/protected classes, methods or variables that contain "master" in the package will be deprecated as well, to avoid spending time modifying the same file again.
  2. Any public/protected Class with "master" in the name will extend the renamed class to maintain only one class while supporting the old class.
  3. Any public/protected method or variable with "master" in the name will directly call the renamed method or variable, for the same reason as above.
  4. Renaming and deprecating class/method/variable must be in 2 separate Pull Requests, in order to be in separate git commits after PR merged.
    Because there will be 2 classes as the result, one with new the name and one with the old name, and the class with new name should be the successor, only in this way can preserve the git file history in the renamed class.

In detail:

  1. Copy the class file and keep it in another place.
  2. Replace Master word with ClusterManager in the class name. This is done by the Rename refactoring feature of IntelliJ IDEA, so that both the definition and reference can be renamed.
  3. replace master with clusterManager in the method or variable name, using the same way as above.
  4. Create a Pull Request for the above changes, so that the file renaming can be tracked in the git history.
  5. Wait until the PR merged.
  6. Paste the original master class file back.
  7. Remove all the existing implementation in the old classes, except any public/protected method or variable that contains "master" name. Make the old class extends the new renamed class.
    For class, adding all the constructors of the super class.
    For the method or variable contains inclusive name, change its implementation to call the corresponding ones in super class.
  8. Add @Deprecated annotation and @deprecated Javadoc tag for the class and method that contains "master" in the name.
  9. If the renamed class has unit test, copy the unit test to test the deprecated class.
  10. Create another Pull Request for the above changes.
  11. Backport all the changes to 2.x branch.

Describe alternatives you've considered
None.

Additional context
The regex to filter the lines with public or protected class definition contains "master" terminology:
(public|protected)(.)+(class|interface)\s(\w)*Master
Totally 11 classes, excluding the classes in the package that covered by issue #3542
7 methods and 4 variables in these classes having "master" in the names.

1 Class/Interface to be renamed:
sever:
interface LocalNodeMasterListener
final MasterNodeChangePredicate
NotMasterException
NoMasterBlockService
UnsafeBootstrapMasterCommand
MasterService
MasterNotDiscoveredException
RestMasterAction
test/framework:
FakeThreadPoolMasterService
BlockMasterServiceOnMaster
BusyMasterServiceDisruption

2 Method/variable to be renamed:
in interface LocalNodeMasterListener:
void onMaster();
void offMaster();

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)

In class FakeThreadPoolMasterService:
int getFakeMasterServicePendingTaskCount()

@tlfeng tlfeng added enhancement Enhancement or improvement to existing feature or request untriaged labels Jun 8, 2022
@tlfeng tlfeng changed the title Deprecate public or protected class that contains 'master' terminology Deprecate public or protected class that contains "master" terminology in 'server' and 'test/framework' directory Jun 8, 2022
@tlfeng tlfeng added deprecate v2.1.0 Issues and PRs related to version 2.1.0 and removed untriaged labels Jun 8, 2022
@tlfeng tlfeng added v2.2.0 and removed v2.1.0 Issues and PRs related to version 2.1.0 labels Jun 23, 2022
@tlfeng
Copy link
Collaborator Author

tlfeng commented Jul 19, 2022

Progress update on 07/19/2022:
Completed:
The following classes that have been deprecated and alternative classes have been created:

  • in sever directory:
    interface LocalNodeMasterListener
    final MasterNodeChangePredicate
    NotMasterException
    NoMasterBlockService
    UnsafeBootstrapMasterCommand
    MasterNotDiscoveredException
    RestMasterAction

The work is done by the following PRs:
Rename classes: #3619 (backport to 2.x branch in #3870)
Add back the classes in old name and deprecate: #3871 (backport to 2.x branch in #3914)

Most public methods and variables (except abstract methods) are renamed and deprecated in PR #4006.

Remaining:
The following classes are pending rename and deprecate:

  • in sever directory:
    MasterService
  • in test/framework directory:
    FakeThreadPoolMasterService
    BlockMasterServiceOnMaster
    BusyMasterServiceDisruption

The following abstract methods are pending rename and deprecate:

  • in interface LocalNodeMasterListener:
    void onMaster();
    void offMaster();

1 Reason of the class MasterService is not deprecated:
There is a public method that has return value in type MasterService,


I haven't got a solution to keep the backwards compatibility of the method getMasterService() while deprecating the class MasterService and encourage using a new class ClusterManagerService.

2 Reason of the 3 classes in test/framework directory are not deprecated:
Deprecating class FakeThreadPoolMasterService is blocked by the class MasterService deprecation, because it extends the class MasterService.
The other 2 classes BlockMasterServiceOnMaster and BusyMasterServiceDisruption are not deprecated to keep the name consistency.

3 Reason of the 2 abstract methods in the interface LocalNodeMasterListener: void onMaster() and void offMaster() are not deprecated:
There is a solution to deprecate them properly: #3688 (comment), but I haven't started yet.

@tlfeng
Copy link
Collaborator Author

tlfeng commented Jul 30, 2022

Progress update on 07/29/2022:
Completed:

  1. The class MasterService is deprecated and renamed by PR Deprecate class 'MasterService' and create alternative class 'ClusterManagerService' #4022 (backport to 2.x branch by PR [Backport 2.x] Deprecate class 'MasterService' and create alternative class 'ClusterManagerService' #4050)
  2. The 3 classes in test/framework directory are deprecated and renamed:
FakeThreadPoolMasterService
BlockMasterServiceOnMaster
BusyMasterServiceDisruption

The PR #4051 renamed the classes, and PR #4058 added old classes back and deprecate.
The corresponding backport PRs for 2.x branch is #4057 and #4068

Remaining:
The abstract methods in the interface will be deprecated with the other one method in issue #3544 (comment) together, using the solution in issue #3688 (comment)

in interface LocalNodeMasterListener:
void onMaster();
void offMaster();

@tlfeng
Copy link
Collaborator Author

tlfeng commented Aug 4, 2022

Progress update on 08/03/2022:
Nothing remains for this issue. 🎉
The above remaining item, the abstract method void onMaster() and void offMaster() have been deprecated and renamed by PR #4121, and backported to 2.x and 2.2 branches by PR #4122 and #4123.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecate enhancement Enhancement or improvement to existing feature or request v2.2.0
Projects
None yet
Development

No branches or pull requests

1 participant