Open aggarwalShivani opened 2 months ago
Also tagging @saikaranam-amazon for review
Here's the background of making these changes to enable calling action from another plugin https://github.com/opensearch-project/notifications/pull/223
I've some concerns around moving the classes out of cross cluster replication package. Ideally either of these 2 options should solve the problem: Option 1: Move the ISM action into ccr plugin. From what i understand, ISM actions can be implemented as an plugin. CCR can take a dependency on ISM and implement the stop action in this repo. Option 2: ISM plugin can take a dependency on CCR and reuse the classes. (We do same for security plugin.)
@Ankit Kala commented on Jul 15, 2024, 7:06 AM PDT:
I've some concerns around moving the classes out of cross cluster replication package.
Ideally either of these 2 options should solve the problem:
Option 1: Move the ISM action into ccr plugin. From what i understand, ISM actions can be implemented as an plugin. CCR can take a dependency on ISM and implement the stop action in this repo.
Option 2: ISM plugin can take a dependency on CCR and reuse the classes. (We do same for security plugin.)
We didn't try Option 1 and it looks promising, and we may not need a proxy transport class then since ISM base doesn't have to know anything about CCR.
@aggarwalShivani some quick notes for trying it out
ISM has this interface, https://github.com/opensearch-project/index-management/blob/dbd2bc25f5e338819237af5a6b35c6c1e35d7171/spi/src/main/kotlin/org.opensearch.indexmanagement.spi/IndexManagementExtension.kt#L16.
You can implement that in ReplicationPlugin, and provide the getISMActionParsers
In build.gradle, take a compileOnly dependency on opensearch-index-management-spi. Probably need extendedPlugins = ['opensearch-index-management']
in opensearchplugin {} block
And a resources/META-INF/services/org.opensearch.indexmanagement.spi.IndexManagementExtension
file that have org.opensearch.replication.ReplicationPlugin
in it for the SPI mechanism
@aggarwalShivani
Have a PR to publish ISM spi so it can be consumed from central repo https://github.com/opensearch-project/index-management/pull/1207
And here's the commit of extend ISM in CCR https://github.com/opensearch-project/cross-cluster-replication/commit/3853901d77da71e772a71ea623ed1e721ded5d37
I tested using your script and can see everything works as expected it actually cannot work out of box, seeing lots of jar hell issue.
@ankitkala I tried the method of using ISM spi, but seems still have the same problem
If we extend ReplicationPlugin with ISM spi, that means CCR can only be installed after ISM installed (a hard dependency introduced), and there are a lot of jar hells, probably not what we want here.
If we create a new plugin in CCR, like CCR-ISM-extension plugin, it probably will also suffer the same problem that one plugin cannot use the TransportAction from another plugin directly. As plugin has their own classloaders, same class become different after being loaded by different classloaders.
So I think it's better to stick with the current approach now. For your concern
I've some concerns around moving the classes out of cross cluster replication package.
It seems to be a 2 way door, moving classes around probably won't introduce any issues between versions. But let me know any concerns.
Hi @ankitkala , Just in case you didn't get a notification for latest reponse by @bowenlan-amzn (as I didn't too)....
We see issues with Option 1 ->
If we extend ReplicationPlugin with ISM spi, that means CCR can only be installed after ISM installed (a hard dependency introduced), and there are a lot of jar hells, probably not what we want here. ... If we create a new plugin in CCR, like CCR-ISM-extension plugin, it probably will also suffer the same problem that one plugin cannot use the TransportAction from another plugin directly. As plugin has their own classloaders, same class become different after being loaded by different classloaders.
Even with option 2, it would reverse the problem on ism. And also, if we reuse classes from one plugin in another, we would see class-loading issue i.e. plugins have their own classloaders, same class become different after being loaded by different classloaders. (explained in https://github.com/opensearch-project/notifications/pull/223 as well)
For your concern
I've some concerns around moving the classes out of cross cluster replication package.
To add in addition to Bowen's comment, in our proposed approach, we've ensured that the current REST API users of CCR would not get impacted functionality-wise by any change. ' And other plugins like notifications, alerting also have some of their classes in common-utils.. Pls let us know your thoughts on this.
@ankitkala I tried the method of using ISM spi, but seems still have the same problem
If we extend ReplicationPlugin with ISM spi, that means CCR can only be installed after ISM installed (a hard dependency introduced), and there are a lot of jar hells, probably not what we want here.
I didn't get this part. All the SPI classes are getting loaded in Opensearch process already, we should just be having it as compile time dependency right?
If we create a new plugin in CCR, like CCR-ISM-extension plugin, it probably will also suffer the same problem that one plugin cannot use the TransportAction from another plugin directly. As plugin has their own classloaders, same class become different after being loaded by different classloaders.
I went through the issue. I still have questions around why we'd want to load same class into JVM twice through different classloaders.
So I think it's better to stick with the current approach now. For your concern
I've some concerns around moving the classes out of cross cluster replication package.
It seems to be a 2 way door, moving classes around probably won't introduce any issues between versions. But let me know any concerns.
I definitely understand the issues with the integration and if we really need to move classes out to common utils, we'll do that. But before we do that, i just want to ensure that we've exhausted all options here :-)
Hi @ankitkala , Thanks for your feedback..
I went through the issue. I still have questions around why we'd want to load same class into JVM twice through different classloaders.
I'll try to answer this from my understanding and @bowenlan-amzn could add-on/correct me as needed :)
In the notifications example as well as in the case of this PR, the flow is - ism plugin needs to invoke actions from other plugins, for ex. SendNotificationAction or TransportStopIndexReplicationAction. These are transport actions in their respective plugins, which accept request of fixed type like StopIndexReplicationRequest.
To invoke these transport action, ism would need to first construct request object of the required type (ex. StopIndexReplicationRequest), invoke the action from the other plugin and finally expect the response of type returned by that action i.e. the request class object is created in ism, but it has to be used in ccr code.
OpenSearch loads each plugin by different class loader separately, which means the same class StopIndexReplicationRequest would have to be recreated/reloaded into ccr's classloader - which causes issues. Same case with response classes too.
have to be recreated/reloaded into ccr's classloader
Hey @aggarwalShivani, what i don't understand is why we need to load the ccr classes at runtime. Compile time dependency makes sense, but at runtime, these classes are already loaded into the opensearch jvm(even if its through a different classloader).
@Ankit Kala commented on Jul 29, 2024, 8:23 PM PDT:
have to be recreated/reloaded into ccr's classloader
Hey @aggarwalShivani, what i don't understand is why we need to load the ccr classes at runtime. Compile time dependency makes sense, but at runtime, these classes are already loaded into the opensearch jvm(even if its through a different classloader).
Valid question. What you are suggesting is to take the stop replication request from a compile dependency of ISM. And we should have that request class at runtime if CCR plugin is also installed in OpenSearch.
@ankitkala can your team help our contributor here to enable this workflow you are thinking? I'm not sure if CCR is publishing any jar for now.
Note, based on how class loaded in OpenSearch, we will need the CCR jar load from opensearch core I guess, otherwise it probably not recognizable from other plugin as it's loaded by CCR plugin classloader?
@Ankit Kala commented on Jul 24, 2024, 8:45 AM PDT:
@ankitkala I tried the method of using ISM spi, but seems still have the same problem
If we extend ReplicationPlugin with ISM spi, that means CCR can only be installed after ISM installed (a hard dependency introduced), and there are a lot of jar hells, probably not what we want here.
I didn't get this part. All the SPI classes are getting loaded in Opensearch process already, we should just be having it as compile time dependency right?
If we define CCR as extended plugin of ISM (refer here), when we install the plugin, it hit this check of jarhell https://github.com/opensearch-project/OpenSearch/blob/05fb780a3aec3bfb46c0cb76c65e340b11d42def/server/src/main/java/org/opensearch/plugins/PluginsService.java#L679
It requires that the extension plugin CCR to not have any duplicate dependencies as its extended plugin ISM.
@ankitkala can your team help our contributor here to enable this workflow you are thinking? I'm not sure if CCR is publishing any jar for now.
Note, based on how class loaded in OpenSearch, we will need the CCR jar load from opensearch core I guess, otherwise it probably not recognizable from other plugin as it's loaded by CCR plugin classloader?
Hi @ankitkala, Pls let us know if you have any feedback/views on this 🙂
@ankitkala can your team help our contributor here to enable this workflow you are thinking? I'm not sure if CCR is publishing any jar for now. Note, based on how class loaded in OpenSearch, we will need the CCR jar load from opensearch core I guess, otherwise it probably not recognizable from other plugin as it's loaded by CCR plugin classloader?
Hi @ankitkala, Pls let us know if you have any feedback/views on this 🙂
Hi, unfortunately we don't have bandwidth to pick this up. In spirit of moving forward, let's go ahead with the proposed changes.
Description
This is related to the feature unfollow-action #726 and is the 2nd PR in continuation of the proposed solution mentioned in common-utils-PR. Detailed information is explained there.
Change description:
Issues Resolved
Related Issues unfollow-action #726
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.