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 implement interface 'o.o.cluster.LocalNodeMasterListener' should not have to implement abstract method 'onClusterManager()' and 'offClusterManager()' #3688

Closed
tlfeng opened this issue Jun 24, 2022 · 5 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 24, 2022

Describe the bug
Similar issue: #3663 and #3683
Introduced by PR #2519 / commit 79eb3b0, which aims to resolve issue #1548

The problem:
2 abstract methods onMaster() and offMaster() of interface LocalNodeMasterListener got renamed directly.
It broke the backwards compatibility that all classes that implements the interface have to implement the methods in the new name.

It causes build failure for plugins, such as https://github.com/opensearch-project/index-management/blob/b6e7d35c4fe4b64a41ef9e45d0cafcdfe0289042/src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/IndexStateManagementHistory.kt#L85

A solution right now is to revert the changes, which means change back the method name onMaster() and offMaster() in the interface LocalNodeMasterListener.

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

java.lang.AbstractMethodError: Receiver class org.opensearch.indexmanagement.indexstatemanagement.IndexStateManagementHistory does not define or inherit an implementation of the resolved method 'abstract void onClusterManager()' of interface org.opensearch.cluster.LocalNodeMasterListener.

Expected behavior
Index management plugin should successfully be built with OpenSearch 2.1.0

Additional context
Add any other context about the problem here.

@tlfeng tlfeng added enhancement Enhancement or improvement to existing feature or request untriaged labels Jun 24, 2022
@tlfeng tlfeng added bug Something isn't working and removed enhancement Enhancement or improvement to existing feature or request untriaged labels Jun 24, 2022
@tlfeng
Copy link
Collaborator Author

tlfeng commented Jun 24, 2022

The solution has been applied to 2.1 branch by the PR #3687 / commit 8aadcc0, the problematic change has been reverted.

@tlfeng tlfeng changed the title [BUG] Class that implements class 'o.o.cluster.support.master.LocalNodeMasterListener' should not have to implement abstract method 'onClusterManager()' and 'offClusterManager()' [BUG] Classes that implement class 'o.o.cluster.support.master.LocalNodeMasterListener' should not have to implement abstract method 'onClusterManager()' and 'offClusterManager()' Jun 24, 2022
@tlfeng tlfeng added the v2.1.0 Issues and PRs related to version 2.1.0 label Jun 24, 2022
@tlfeng
Copy link
Collaborator Author

tlfeng commented Jun 24, 2022

Close the issue. As the onMaster() and offMaster() method name have been restored in all branches.
PR #3686 for main branch, and PR #3693 for 2.x branch.

@tlfeng tlfeng closed this as completed Jun 24, 2022
@tlfeng tlfeng changed the title [BUG] Classes that implement class 'o.o.cluster.support.master.LocalNodeMasterListener' should not have to implement abstract method 'onClusterManager()' and 'offClusterManager()' [BUG] Classes that implement interface 'o.o.cluster.support.master.LocalNodeMasterListener' should not have to implement abstract method 'onClusterManager()' and 'offClusterManager()' Jun 24, 2022
@tlfeng tlfeng changed the title [BUG] Classes that implement interface 'o.o.cluster.support.master.LocalNodeMasterListener' should not have to implement abstract method 'onClusterManager()' and 'offClusterManager()' [BUG] Classes that implement interface 'o.o.cluster.LocalNodeMasterListener' should not have to implement abstract method 'onClusterManager()' and 'offClusterManager()' Jul 20, 2022
@tlfeng
Copy link
Collaborator Author

tlfeng commented Jul 20, 2022

I have got a solution from @andrross to deprecate the abstract methods onMaster() and offMaster() properly.

public interface Listener {
    void onMaster();
    void offMaster();

    default void onClusterManager() {
        onMaster();
    }
    default void offClusterManager() {
        offMaster();
    }
}

Introducing a new abstract method is a breaking change, and adding a new method with a default implementation is not a breaking change.

In addition, a new unit test is added by PR #3871:

public void testDeprecatedLocalNodeMasterListenerCallbacks() {

to validate the backwards compatibility for the abstract methods onMaster() and offMaster().

@tlfeng
Copy link
Collaborator Author

tlfeng commented Aug 3, 2022

Specifically, the solution provided by @andrross for deprecating the abstract methods in interface LocalNodeMasterListner is:

@deprecated
public interface LocalNodeMasterListener extends LocalNodeClusterManagerListener {
    void onMaster();

    void offMaster();

    @Override
    default void onClusterManager() {
        onMaster()
    }

    @Override
    default void offClusterManager() {
        offMaster();
    }
}

public interface LocalNodeClusterManagerListener extends ClusterStateListener {
    void onClusterManager();

    void offClusterManager();
}

All places that take a LocalNodeMasterListener instance can be changed to take a LocalNodeClusterManagerListener. Developers can still use LocalNodeMasterListener everywhere and it will still work as-is, though they’ll get deprecation warnings.

@tlfeng
Copy link
Collaborator Author

tlfeng commented Aug 4, 2022

The final solution in PR #4121 has been merged, and from version 2.2.0, developers can use onClusterManager() and offClusterManager() to replace onMaster() and offMaster()
For more detail: #3543 (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