Remove @ExperimentalApi annotations from star-tree related code #18521
Conversation
Signed-off-by: SuBin Ahn <[email protected]>
happysubin
requested review from
a team,
Bukhtawar,
CEHENKLE,
Rishikesh1159,
VachaShah,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
github-actions
bot
added
_No response_
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #18521 +/- ##
============================================
+ Coverage 72.64% 72.72% +0.08%
- Complexity 68070 68111 +41
============================================
Files 5537 5537
Lines 313404 313381 -23
Branches 45476 45472 -4
============================================
+ Hits 227658 227915 +257
+ Misses 67241 66900 -341
- Partials 18505 18566 +61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sandeshkr419 |
| * Collects one or more @{@link StarTreeNode}'s | ||
| */ | ||
| @ExperimentalApi | ||
| @PublicApi(since = "2.18.0") |
There was a problem hiding this comment.
Can we get rid of PublicApi annotation as well? Classes although public, still shouldn't be annotated PublicApi
I think you will get a trail of classes that might fail to build because they are still marked experimental which is okay to remove experimental as well.
There was a problem hiding this comment.
Thank you for your review !
However, when I remove the @PublicApi annotation and run the build, I get the following error
error: The element org.opensearch.search.startree.StarTreeNodeCollector is part of the public APIs but is not marked as @PublicApi, @ExperimentalApi or @DeprecatedApi (referenced by org.opensearch.search.startree.filter.DimensionFilter)
How can I resolve this?
There was a problem hiding this comment.
org.opensearch.search.startree.filter.DimensionFilter too is marked as public api. Can we remove publicApi from the chain and see what's the actual class that could be causing this error ?
There was a problem hiding this comment.
@bharath-techie
I haven’t pushed yet,
I continue removing the PublicApi annotation starting from DimensionFilter,
I end up removing it from the following related classes.
DimensionFilter, DimensionFilterMapper, StarTreeNode, StarTreeFilter, and StarTreeQueryContext
Eventually, this leads to the following error
error: The element org.opensearch.search.startree.StarTreeQueryContext is part of the public APIs but is not marked as @PublicApi, @ExperimentalApi or @DeprecatedApi (referenced by org.opensearch.index.query.QueryShardContext)
There was a problem hiding this comment.
I don't see good options other than marking the chain with public API since we have a reference in QueryShardContext.
Any thoughts ?
There was a problem hiding this comment.
I strongly don't like the idea of chaining up all the classes as @PublicApi as a workaround.
Thinking out loud some alternatives:
How about introducing a CompositeQueryContext with @PublicApi annotation. Its implementation StarTreeQueryContext can be an internal api or with no annotations. QueryShardContext will then hold CompositeQueryContext instead of StarTreeQueryContext.
I think this can provide a good abstraction since star-tree is also an implementation of composite index. Introducing CompositeQueryContext can hide the star-tree related details and also pave way for future implementations similar to star-tree without having to bundle up query context with additional information.
There was a problem hiding this comment.
This doesn't solve the problem by the way , still the chain of StarTreeQueryContext needs to be PublicApi since CompositeQueryContext is public API.
There was a problem hiding this comment.
For example if we do the above change , we still have the following chain that needs to be solved
CompositeDataCubeFieldType --> StarTreeQueryContext
Dimension -> CompositeDataCubeFieldType
DimensionDataType -> Dimension
Metric -> CompositeDataCubeFieldType
MetricStat -> Metric
And there is an additional chain :
CompositeIndexSettings --> IndicesService
But it does remove the chain in same package chain - DimensionFilter, StarTreeFilter, DimensionFilterMapper.
There was a problem hiding this comment.
Basically exposing a sub-set of interfaces as @PublicAPI is still better (may not be ideal - and we can increment on this) than exposing all the implementations as @PublicAPI.
There was a problem hiding this comment.
I think this PR got stalled.
@happysubin if you are still interested in closing this PR, lets try doing subset of classes as @sandeshkr419 suggested here in #18521 (comment).
|
Thanks a lot for contributing to this PR @happysubin . We also can remove experimentalApi from star tree indexing side of code - https://github.com/opensearch-project/OpenSearch/tree/main/server/src/main/java/org/opensearch/index/compositeindex |
|
This PR is stalled because it has been open for 30 days with no activity. |
opensearch-trigger-bot
bot
added
stalled
|
This PR is stalled because it has been open for 30 days with no activity. |
opensearch-trigger-bot
bot
added
stalled
|
This PR is stalled because it has been open for 30 days with no activity. |
3 participants
Description
Removal of ExperimentalApi annotations in star-tree related code files
Related Issues
Resolves #18499
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.