Added new baseclasses in opensearch for ccr classloader issue #13615
Conversation
Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com>
aggarwalShivani
requested review from
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
dreamer-89,
gbbafna,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
tlfeng and
VachaShah
as code owners
Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com>
|
❌ Gradle check result for 4aeb5fa: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Hi @dblock and @bowenlan-amzn, |
|
❌ Gradle check result for 71852dd: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
First time saw such error You want to change the file permission of these 2 files and push up |
Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com>
|
❌ Gradle check result for 48738a6: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com>
Signed-off-by: aggarwalShivani <shivani.aggarwal@nokia.com>
|
❕ Gradle check result for e35b20b: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13615 +/- ##
============================================
+ Coverage 71.42% 71.55% +0.13%
- Complexity 59978 61145 +1167
============================================
Files 4985 5054 +69
Lines 282275 287301 +5026
Branches 40946 41619 +673
============================================
+ Hits 201603 205582 +3979
- Misses 63999 64748 +749
- Partials 16673 16971 +298 ☔ View full report in Codecov by Sentry. |
|
❌ Gradle check result for 1bf2e04: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Description
(Apologies in advance, for a long description ahead ;) )
Background:
This change is related to a new feature being added in ism - unfollow-action #726 - integrating ccr and ism plugins.
It requires ism plugin to invoke stop-replication action from ccr. As both ccr and ism need to invoke some common code, the common code could be moved to common-utils, as this has been done previously for other plugins too. For ex. notifications plugin.
However, sharing of libraries also leads to a type-cast and class-loading issue - previously seen with notification plugin.
Some highlights from that PR's description ->
OpenSearch loads each plugin by different class loader separately. When two plugins need to communicate by transport action, it’s common to put request and response class into a common module shared by both.
However, as per the forum post, this may cause problem because OpenSearch does "optimization to avoid serialization" for local request between plugins on same JVM.
As suggested by @bowenlan-amzn, this means as followed for notification and other plugins, the request object needs to be of a higher-level class from opensearch-core like ActionRequest and later be recreated into required StopIndexReplicationRequest type.
To achieve this, TransportStopIndexReplicationAction's doExecute method needs to be overridden, to accept a request of different type (a class from opensearch-core) instead of StopIndexReplicationRequest.
Issue:
extends ClusterManagerNodeRequest<Request>No class in opensearch-core meets this condition. So we cannot override the method with any existing class from the opensearch-core.
Change description:
Because the generics constraints are very strict for ClusterManagerNodeRequest and TransportClusterManagerNodeAction, I propose to introduce two new classes in Opensearch, similar to ClusterManagerNodeRequest and TransportClusterManagerNodeAction, but primarily change only in the generics.
This can be used in future, by other plugins too, that use TransportClusterManagerNodeAction and need to use a different request type.
Proposed solution:
This is an initial PR catering to point 1. I've not yet worked on the test coverage part as I would request reviewers to give feedback on this approach, so we can proceed accordingly.
Related Issues
It is a pre-req required for opensearch-project/index-management#726.
Check List
[ ] Public documentation issue/PR createdBy 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.