opensearch-project / OpenSearch

🔎 Open source distributed and RESTful search engine.
https://opensearch.org/docs/latest/opensearch/index/
Apache License 2.0
9.87k stars 1.84k forks source link

[BUG] CCR 3.0 build failing due to BaseNodeRequest #4938

Open ankitkala opened 2 years ago

ankitkala commented 2 years ago

We have a CCR build failing while trying to bump the CCR plugin to 3.0 with following error.

> Task :compileKotlin FAILED
e: /Volumes/ws/cross-cluster-replication/src/main/kotlin/org/opensearch/replication/action/stats/NodeStatsRequest.kt: (14, 44): Unresolved reference: BaseNodeRequest
e: /Volumes/ws/cross-cluster-replication/src/main/kotlin/org/opensearch/replication/action/stats/NodeStatsRequest.kt: (19, 26): Unresolved reference: BaseNodeRequest
e: /Volumes/ws/cross-cluster-replication/src/main/kotlin/org/opensearch/replication/action/stats/NodeStatsRequest.kt: (21, 43): Too many arguments for public constructor Any() defined in kotlin.Any
e: /Volumes/ws/cross-cluster-replication/src/main/kotlin/org/opensearch/replication/action/stats/NodeStatsRequest.kt: (26, 5): 'writeTo' overrides nothing
e: /Volumes/ws/cross-cluster-replication/src/main/kotlin/org/opensearch/replication/action/stats/NodeStatsRequest.kt: (27, 15): Unresolved reference: writeTo
e: /Volumes/ws/cross-cluster-replication/src/main/kotlin/org/opensearch/replication/action/stats/TransportFollowerStatsAction.kt: (37, 75): Type argument is not within its bounds: should be subtype of 'TransportRequest!'
e: /Volumes/ws/cross-cluster-replication/src/main/kotlin/org/opensearch/replication/action/stats/TransportFollowerStatsAction.kt: (38, 103): Callable reference resolution ambiguity: 
public constructor NodeStatsRequest() defined in org.opensearch.replication.action.stats.NodeStatsRequest
public constructor NodeStatsRequest(inp: StreamInput) defined in org.opensearch.replication.action.stats.NodeStatsRequest
e: /Volumes/ws/cross-cluster-replication/src/main/kotlin/org/opensearch/replication/action/stats/TransportLeaderStatsAction.kt: (39, 71): Type argument is not within its bounds: should be subtype of 'TransportRequest!'
e: /Volumes/ws/cross-cluster-replication/src/main/kotlin/org/opensearch/replication/action/stats/TransportLeaderStatsAction.kt: (40, 101): Callable reference resolution ambiguity: 
public constructor NodeStatsRequest() defined in org.opensearch.replication.action.stats.NodeStatsRequest
public constructor NodeStatsRequest(inp: StreamInput) defined in org.opensearch.replication.action.stats.NodeStatsRequest

Looks like we've extended BaseNodeRequest and TransportNodesAction for our stats APIs which has been removed here.

CCR extends TransportNodesAction only for stat APIs where we query all the nodes for the stats and aggregate the results.

If i understand correctly, plugins are expected to extend HandledTransportAction instead of TransportNodesAction.
Can someone confirm whether this is correct? Wouldn't all the plugins have to replicate the logic of TransportNodesAction in this case? Or, if there is any alternate suggestion, happy to explore that as well?

PS: Though the BaseNodeRequest class was removed in the commit mentioned above, i can still see this file on main . Not sure if it was added back in some other commit.

ankitkala commented 2 years ago

@nknize

nknize commented 2 years ago

If i understand correctly, plugins are expected to extend HandledTransportAction instead of TransportNodesAction. Can someone confirm whether this is correct?

This is correct;

Wouldn't all the plugins have to replicate the logic of TransportNodesAction in this case? Or, if there is any alternate suggestion, happy to explore that as well?

Replicate what logic specifically? The HandledTransportAction is registering the Action in the same manner as the TransportNodesAction ctor. It's just the latter registers internally during ActionModule initialization and the former registers through the TransportService.

nknize commented 2 years ago

PS: Though the BaseNodeRequest class was removed in the commit mentioned above, i can still see this file on main . Not sure if it was added back in some other commit.

I think you're looking at BaseNodesRequest (with an 's'). BaseNodeRequest is not in main.

ankitkala commented 2 years ago

Replicate what logic specifically?

TransportNodesAction would send the request to multiple nodes based on the input request and return a multi-node response. https://github.com/opensearch-project/OpenSearch/blob/fe3994ccdf62735907bc4834f93c71d97636d150/server/src/main/java/org/opensearch/action/support/nodes/TransportNodesAction.java#L244-L294

nknize commented 2 years ago

TransportNodesAction would send the request to multiple nodes ...

+1, yes you're correct. This is the only class that performs this AsyncAction and it was only used internally in core for mechanisms like Node Stats, HotThead, Snapshot status. Since CCR is effectively mimicking the stats API it makes sense it extended this class.

So, in this pre-extensions era we could do one of two things:

  1. re-label the abstract TransportNodesAction as opensearch.api and begin supporting BWC on this class.
  2. Create a new o.opensearch.action.api` package and add a simple API shim that external plugins can extend.

@saratvemulapalli are we at any kind of state to easily begin implementing the API in the java sdk so CCR could use it for implementing a concrete TransportNodesAction?

If not it seems like 1. or 2. are the easiest and safest options?

dbwiddis commented 2 years ago

Create a new o.opensearch.action.api package and add a simple API shim that external plugins can extend.

Can you elaborate on what you mean here? Since extensions operate (by default) in a separate JVM, extending a class buys you some things but you still have to communicate the results over either transport or REST. It seems more reasonable to me to create a new package designed for this type of data, in preference to grabbing bits and pieces from the codebase.