[Backport 2.x] Make IndexStoreListener a pluggable interface

[opensearch-trigger-bot]

Backport 9b7681c from #16583.

[jed326]

@reta I see detect breaking changes is failing, I think because I removed NodeEnvironment.IndexStoreListener which was public, but also marked as @opensearch.internal so it should be fine to removed/replace -- any pointers?

I don't think it's related to #16585 either

[jed326]

Ah and it's because I am also changing the constructor here to the new class:

OpenSearch/server/src/main/java/org/opensearch/env/NodeEnvironment.java

Line 309 in 9b7681c

public NodeEnvironment(Settings settings, Environment environment, IndexStoreListener indexStoreListener) throws IOException {

Should I move IndexStoreListener back into NodeEnvironment, at least on 2.x?

IMO this isn't really a breaking change because the class that was modified is marked as internal and transitively that means the constructor that was using said internal class is also internal. I do get that technically it is breaking though since we are changing the public constructor / removing the public classes.

[jed326]

@sohami @andrross curious to hear your opinions on this as well

[andrross]

@jed326 The breaking changes check is pretty strict and we can't override it because it will just keep failing for all subsequent PRs. Can you create an empty interface/class NodeEnvironment.IndexStoreListener on 2.x that just extends the new public class? And also add back the ctor parameter even if it is unused.

[github-actions]

✅ Gradle check result for b602dfd: SUCCESS

[jed326]

Thanks @andrross , that makes sense let me give that a try. I'm not entirely sure if extending the new interface will work but worst case we can just leave the untouched old one around too.

[reta]

@sohami @andrross curious to hear your opinions on this as well

@jed326 @andrross One sec, looking, we should not fail for non-annotated classes.

[codecov]

Codecov Report

Attention: Patch coverage is 86.48649% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.86%. Comparing base (a957ded) to head (ee72da1).
Report is 4 commits behind head on 2.x.

Files with missing lines	Patch %	Lines
...org/opensearch/index/store/IndexStoreListener.java	80.95%	4 Missing ⚠️
.../main/java/org/opensearch/env/NodeEnvironment.java	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x   #16594      +/-   ##
============================================
+ Coverage     71.75%   71.86%   +0.11%     
- Complexity    65322    65379      +57     
============================================
  Files          5313     5314       +1     
  Lines        305109   305139      +30     
  Branches      44456    44458       +2     
============================================
+ Hits         218920   219291     +371     
+ Misses        67970    67614     -356     
- Partials      18219    18234      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[reta]

@jed326 The breaking changes check is pretty strict and we can't override it because it will just keep failing for all subsequent PRs. Can you create an empty interface/class NodeEnvironment.IndexStoreListener on 2.x that just extends the new public class? And also add back the ctor parameter even if it is unused.

@jed326 the @andrross suggestion is correct (or we could place IndexStoreListener back into NodeEnvironment), NodeEnvironment is a public API and it is being modified in non-compatible way.

[jed326]

Alright I brought back those old interfaces / constructors and marked them as @Deprecated, seems like breaking changes is passing now. Thanks @andrross @reta !

[github-actions]

❕ Gradle check result for ee72da1: 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.

[andrross]

@reta was this change required? I thought we exempted the constructors from the check?

[reta]

@reta was this change required? I thought we exempted the constructors from the check?

@andrross not by default, but if annotated with @InternalApi (not the case)