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

[BUG] Classes that extend class 'o.o.action.support.master.TransportMasterNodeAction' should not have to implement abstract method 'clusterManagerOperation()' #3683

Closed
tlfeng opened this issue Jun 23, 2022 · 4 comments
Labels
backwards-compatibility bug Something isn't working v2.1.0 Issues and PRs related to version 2.1.0

Comments

@tlfeng
Copy link
Collaborator

tlfeng commented Jun 23, 2022

Describe the bug
Similar issue: #3663 and #3688
Introduced by PR #3617 / commit 60d7a09, which aims to resolve issue #3542

The problem:
To deprecate public methods that has "master" in the name, I created an alternative method clusterManagerOperation(...) to replace masterOperation(...), which is an abstract method.
Location: https://github.com/opensearch-project/OpenSearch/blob/157e69330891b8133d56ab993b5022f2e8c6ee04/server/src/main/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeAction.java
It makes classes that extend the class TransportMasterNodeAction have to implement the newly added abstract method clusterManagerOperation(...), and it breaks the backwards compatibility.

The solution is to revert the changes to remove the abstract method clusterManagerOperation(...).

To Reproduce
Build k-NN plugin with OpenSearch 2.1.0 (the code at https://github.com/opensearch-project/OpenSearch/tree/304d830275fdbe5f8095a7285ca66266987c727a)

java.lang.AbstractMethodError: Missing implementation of resolved method 'abstract void clusterManagerOperation(org.opensearch.action.support.clustermanager.ClusterManagerNodeRequest, org.opensearch.cluster.ClusterState, org.opensearch.action.ActionListener)' of abstract class org.opensearch.action.support.clustermanager.TransportClusterManagerNodeAction. » at org.opensearch.action.support.clustermanager.TransportClusterManagerNodeAction.clusterManagerOperation(TransportClusterManagerNodeAction.java:135) » at org.opensearch.action.support.clustermanager.TransportClusterManagerNodeAction$AsyncSingleAction.lambda$doStart$3(TransportClusterManagerNodeAction.java:223)

Expected behavior
k-NN plugin should successfully be built with OpenSearch 2.1.0

Additional context
Add any other context about the problem here.

@tlfeng tlfeng added bug Something isn't working untriaged labels Jun 23, 2022
@tlfeng tlfeng changed the title [BUG] Class that extends class 'o.o.action.support.master.MasterNodeRequest' should not have to implement abstract method 'clusterManagerOperation()' [BUG] Class that extends class 'o.o.action.support.master.TransportMasterNodeAction' should not have to implement abstract method 'clusterManagerOperation()' Jun 23, 2022
@tlfeng
Copy link
Collaborator Author

tlfeng commented Jun 24, 2022

The solution has been applied to 2.1 branch by the PR #3682, the problematic change has been reverted.

@tlfeng tlfeng added backwards-compatibility v2.1.0 Issues and PRs related to version 2.1.0 and removed untriaged labels Jun 24, 2022
@tlfeng tlfeng changed the title [BUG] Class that extends class 'o.o.action.support.master.TransportMasterNodeAction' should not have to implement abstract method 'clusterManagerOperation()' [BUG] Classes that extend class 'o.o.action.support.master.TransportMasterNodeAction' should not have to implement abstract method 'clusterManagerOperation()' Jun 24, 2022
@tlfeng
Copy link
Collaborator Author

tlfeng commented Jun 27, 2022

Close the issue. The masterOperation() method name have been restored in all branches.
PR #3681 for main branch, and PR #3714 for 2.x branch.

@tlfeng tlfeng closed this as completed Jun 27, 2022
@tlfeng
Copy link
Collaborator Author

tlfeng commented Jul 20, 2022

I have got a solution from @andrross to properly deprecate the abstract method 'masterOperation()'.

@Deprecated("override doSomethingWithClusterManager instead")
void doSomethingWithMaster() {
    throw new UnsupportedOperationException("Must be overridden");
}

void doSomethingWithClusterManager() {
    doSomethingWithMaster();
}

The below is wrong because it will break any child classes.

@Deprecated("override doSomethingWithClusterManager instead")
abstract void doSomethingWithMaster();

abstract void doSomethingWithClusterManager();

@tlfeng
Copy link
Collaborator Author

tlfeng commented Aug 4, 2022

The final solution in PR #4032 has been merged, and from version 2.2.0, developers can use clusterManagerOperation() to replace masterOperation().
For more detail: #3542 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-compatibility bug Something isn't working v2.1.0 Issues and PRs related to version 2.1.0
Projects
None yet
Development

No branches or pull requests

1 participant