-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Make IndexStoreListener a pluggable interface #16583
Conversation
b50c474
to
8c4fd86
Compare
@andrross @bugmakerrrrrr @sohami could you review this PR as it is related to #11443? |
❌ Gradle check result for 8c4fd86: 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? |
8c4fd86
to
829b470
Compare
server/src/main/java/org/opensearch/index/store/IndexStoreListener.java
Outdated
Show resolved
Hide resolved
829b470
to
ceeb08a
Compare
❌ Gradle check result for ceeb08a: 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? |
server/src/main/java/org/opensearch/index/store/IndexStoreListener.java
Outdated
Show resolved
Hide resolved
ceeb08a
to
65497a0
Compare
❌ Gradle check result for 65497a0: 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? |
65497a0
to
2de2403
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16583 +/- ##
============================================
+ Coverage 72.04% 72.11% +0.07%
- Complexity 65093 65102 +9
============================================
Files 5314 5315 +1
Lines 303541 303572 +31
Branches 43921 43925 +4
============================================
+ Hits 218683 218935 +252
+ Misses 66946 66672 -274
- Partials 17912 17965 +53 ☔ View full report in Codecov by Sentry. |
lgtm. Will wait for others to approve as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, once the @PublicApi
tag is added per @reta's comment
Signed-off-by: Jay Deng <[email protected]>
2de2403
to
ca32a2f
Compare
❕ Gradle check result for ca32a2f: 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. |
Signed-off-by: Jay Deng <[email protected]> (cherry picked from commit 9b7681c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Make IndexStoreListener a pluggable interface (#16583) Signed-off-by: Jay Deng <[email protected]> (cherry picked from commit 9b7681c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Resolve breaking changes Signed-off-by: Jay Deng <[email protected]> --------- Signed-off-by: Jay Deng <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Jay Deng <[email protected]>
Description
I have a use case where I have a custom file cache implementation and in order to not have dangling entries in my fc I need to implement
IndexStoreListener
to clean up my file cache when shards move off a node. Today that is not possible asIndexStoreListener
is an@opensearch.internal
interface and not pluggable, which this PR changes.Long term I think the solution is to make the FileCache itself pluggable, but that is much more complicated and anyways would require us to expose this interface as pluggable, so I believe this is a useful intermediary step.
The other thing to note is in
Node.java
I am now creating theCompositeIndexStoreListener
regardless of if itisSearchNode()
or not as it should be up to the specific listener implementations to determine how/when to perform their action.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.