[BUG] Compatibility issue with plugins about class 'org.opensearch.action.support.master.AcknowledgedResponse'

[tlfeng]

Describe the bug
Similar issue: #3683 and #3688
Introduced by PR #3593 / commit 223d472, which aims to resolve issue #3542

The change of class org.opensearch.action.support.master.AcknowledgedResponse causes compatibility issue with plugins that uses this class.

For example:
This is the code in security that causes build failure:
https://github.com/opensearch-project/security/blob/f431ec2201e1466b7c12528347a1f54cf64387c9/src/main/java/org/opensearch/security/tools/SecurityAdmin.java#L518

The problem is the variable definition is in type of a subclass, while an object in superclass is assigned to the variable.

The current AcknowledgedResponse in master package extends the same name class in clustermanager class
https://github.com/opensearch-project/OpenSearch/blob/223d472e6d8ea4454cb05fba7271d213430a1a4e/server/src/main/java/org/opensearch/action/support/master/AcknowledgedResponse.java
In the meantime, I changed the return value type of "putSettings" method to the class in "cluster manager" package.
While security plugin is using the AcknowledgedResponse class in master package.

To Reproduce

1. Build OpenSearch 2.1.0 Snapshot using the code https://github.com/opensearch-project/OpenSearch/tree/79cabafebc74e8379f7c854657693ca3b03a3247
2. Build security plugin or k-NN plugin with the above OpenSearch build artifact.
3. Plugins can't be built.

Expected behavior
The current plugins can be built correctly without making name change to OpenSearch Java API.

Additional context
A build failure in CCR plugin (comes from opensearch-project/cross-cluster-replication#398):
/home/runner/work/cross-cluster-replication/cross-cluster-replication/src/main/kotlin/org/opensearch/replication/action/autofollow/TransportAutoFollowClusterManagerNodeAction.kt: (48, 1): Class 'TransportAutoFollowClusterManagerNodeAction' is not abstract and does not implement abstract base class member protected/*protected and package*/ abstract fun clusterManagerOperation(p0: AutoFollowClusterManagerNodeRequest!, p1: ClusterState!, p2: ActionListener<AcknowledgedResponse!>!): Unit defined in org.opensearch.action.support.master.TransportMasterNodeAction

Solution:
Restore the change for this class to keep the compatibility.

To restore the class org.opensearch.action.support.master.AcknowledgedResponse to its original appearance (https://github.com/opensearch-project/OpenSearch/blob/2.0.1/server/src/main/java/org/opensearch/action/support/master/AcknowledgedResponse.java)
I need to do in 2 PRs to keep the git history of the file:

1. Remove the current class AcknowledgedResponse from package org.opensearch.action.support.master
2. Move the class AcknowledgedResponse from package org.opensearch.action.support.clustermanager to org.opensearch.action.support.master (https://github.com/opensearch-project/OpenSearch/blob/7005b9eb42f66912507b8f973b79734c260bdfe9/server/src/main/java/org/opensearch/action/support/clustermanager/AcknowledgedResponse.java)

[tlfeng]

The problem will be resolved for now after the backport PR #3670 merged.
I will think of the correct way to deal with the class AcknowledgedResponse and related classes.

[andrross]

Since Java doesn't really offer a clean way to change only the return type of a method in a non-breaking way, it's my opinion that we can probably do this as a breaking change. We'll have to proactively reach out to all the plugins in opensearch-project, but it should be as simple as a find-and-replace in the import statement, i.e. replace

import org.opensearch.action.support.master.AcknowledgedResponse;

with

import org.opensearch.action.support.clustermanager.AcknowledgedResponse;

At a minimum we should identify all usages across all repos and open issues, or even just open PRs with the renamed import statement.

[tlfeng]

There are other 3 classes that have relationship with the class AcknowledgedResponse, they will likely be treated in the same way together. They all in the same package org.opensearch.action.support.master.

AcknowledgedRequest
AcknowledgedRequestBuilder
ShardsAcknowledgedResponse

Reason:

• ShardsAcknowledgedResonse
  It's a subclass of AcknowledgedResponse. To keep the consistency, it will be treated in the same way.
  

  OpenSearch/server/src/main/java/org/opensearch/action/support/master/ShardsAcknowledgedResponse.java

  Line 52 in 82ff463

  public abstract class ShardsAcknowledgedResponse extends AcknowledgedResponse {

• AcknowledgedRequest and AcknowledgedRequestBuilder

Because class AcknowledgedRequestBuilder extends class MasterNodeOperationRequestBuilder, and the type parameter Request in MasterNodeOperationRequestBuilder is restricted to MasterNodeRequest (or its subclasses),
the type Request in AcknowledgedRequestBuilder also needs to be of type MasterNodeRequest (or its subclasses).

The below is the class definition of AcknowledgedRequestBuilder and MasterNodeOperationRequestBuilder:

OpenSearch/server/src/main/java/org/opensearch/action/support/master/AcknowledgedRequestBuilder.java

Lines 43 to 49 in 82ff463

	public abstract class AcknowledgedRequestBuilder<
	Request extends AcknowledgedRequest<Request>,
	Response extends AcknowledgedResponse,
	RequestBuilder extends AcknowledgedRequestBuilder<Request, Response, RequestBuilder>> extends MasterNodeOperationRequestBuilder<
	Request,
	Response,
	RequestBuilder> {

OpenSearch/server/src/main/java/org/opensearch/action/support/master/MasterNodeOperationRequestBuilder.java

Lines 46 to 51 in 6462a54

	public abstract class MasterNodeOperationRequestBuilder<
	Request extends MasterNodeRequest<Request>,
	Response extends ActionResponse,
	RequestBuilder extends MasterNodeOperationRequestBuilder<Request, Response, RequestBuilder>> extends ActionRequestBuilder<
	Request,
	Response> {

The solution to deprecate other classes in this package is to make the class with old (non-inclusive) name be a subclass of the class with new (inclusive) name, so that maintaining one class implementation can support 2 the classes simultaneously, such as MasterNodeRequest:

OpenSearch/server/src/main/java/org/opensearch/action/support/master/MasterNodeRequest.java

Line 47 in 82ff463

public abstract class MasterNodeRequest<Request extends MasterNodeRequest<Request>> extends ClusterManagerNodeRequest<Request> {

The above solution is not applicable to class org.opensearch.action.support.master.AcknowledgedRequest.
Due to Java restriction (https://stackoverflow.com/questions/50646585/type-parameter-t-is-not-within-its-bound-should-extend-mybaseclass/50908178#50908178) (https://docs.oracle.com/javase/tutorial/java/generics/bounded.html), when using "Bounded Type Parameters", if one of the bounds is a class, it must be specified first. That's being said org.opensearch.action.support.master.AcknowledgedRequest needs to extend class MasterNodeRequest directly, but Java doesn't support "Multiple inheritance", which means it can't extend class org.opensearch.action.support.clustermanager.AcknowledgedRequest together.

OpenSearch/server/src/main/java/org/opensearch/action/support/master/AcknowledgedRequest.java

Line 49 in 54364a5

public abstract class AcknowledgedRequest<Request extends MasterNodeRequest<Request>> extends MasterNodeRequest<Request>

I tried to let org.opensearch.action.support.master.AcknowledgedRequest extends org.opensearch.action.support.clustermanager.AcknowledgedRequest only, and there was a compile error in class AcknowledgedRequestBuilder, about type Request in the bound AcknowledgedRequest<Request>, it says Type parameter 'Request' is not within its bound; should extend 'org.opensearch.action.support.master.MasterNodeRequest<Request>'.
I haven't found a feasible solution yet.

[tlfeng]

As mentioned above, the 4 classes AcknowledgedRequest, AcknowledgedRequestBuilder, AcknowledgedResponse, ShardsAcknowledgedResponse in package org.opensearch.action.support.master will not be deprecated in version 2.x, and will be moved to package org.opensearch.action.support.clustermanager directly in a future major version as a breaking change.

[peternied]

@tlfeng How will these other deprecations be handled - is there an issue tracking them?

[andrross]

@peternied Unfortunately there's not a good way to do this change in a backward compatible way, so the approach is that these classes will be deprecated in 2.x and then renamed for 3.0. This means there will be a breaking change for 3.0 that will require a trivial update in any plugin that imports these classes.

[andrross]

Resolving this issue again. The inclusive naming deprecation high level approach is detailed in the blog post here: https://opensearch.org/blog/technical-post/2022/10/Adopting-inclusive-language-across-OpenSearch/

[peternied]

@andrross I've created #4856 to track these classes that need to be moved as a breaking change.