-
Notifications
You must be signed in to change notification settings - Fork 64
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
Add api spec for tiering of indices from hot to warm #368
base: main
Are you sure you want to change the base?
Conversation
@dblock please review, let me know if there is anything else that needs to be added. |
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.
Glad you're adding an API with specs!
Will need working tests as part of the PR, otherwise looking ok.
is there any other tests you are looking for me to be added apart from what i added or just making sure that all tests are successful? |
You'll want to exercise all the new API's query parameters. |
Changes AnalysisCommit SHA: f759a99 API ChangesSummary
ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10273263322/artifacts/1782771613 API Coverage
|
f1d3fd3
to
25b9c8c
Compare
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.
Good progress, close! See below and get all tests to pass.
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, see some relatively minor asks below.
4035e95
to
c8e4f6a
Compare
d07c3a4
to
c6ea1ea
Compare
@dblock should we add a higher version on the tests as the feature flag pr(#454) is blocked on the other fields being added to the spec first? thoughts? another way could be to just enable feature flag per test, not sure if there is a mechanism currently to achieve that? |
36dd9d6
to
4a650e9
Compare
@dblock raised a pr in opensearch core package to fix the bug: opensearch-project/OpenSearch#15076 |
@neetikasinghal Looks like the OpenSearch PR got merged, want to finish this and add a 2.17 to CI? |
sure, what would be the @sha256 for 2.17? |
I look at https://hub.docker.com/r/opensearchstaging/opensearch/tags for a tag.
Just add it to .cspell. |
cb350b5
to
464ff4e
Compare
Signed-off-by: Neetika Singhal <[email protected]>
@dblock there would be some more needs for the test to return a status of 200 - e.g. it needs a node with search role configured, else the API would throw a 400. |
Yeah, so this would go into docker-compose. There's a similar ask in #464 for a bunch of configuration. I think we need to find a way not to cram all possible combinations of feature flags and tests into one place, or it will get totally unmanageable. What do you think about adding folders to |
Thats not a bad idea, however these features would be pulled in as well into the main code someday, so there should be an easy way to remove the feature flags and still have the tests working as is. Else, there will be a lot of work to remove the feature flags. So, not sure if we should isolate the features completely. On a separate note: For tiering specifically, its needed for indices to be backed by remote store. Is there a current test that tests the remote store functionality? If not, that is something, which would need to be added for long run also(not just for experimental features.) |
I implemented it in #472 and it got merged. When a feature goes from needing a custom docker-compose to not, we can move the test files to the
AFAIK not. |
@dblock to unblock the specs being present in the package, can we merge this change without the tests and i can add follow-up issues to add the setup for the tests required to run successfully? what do u think? |
I'd rather not. What's stopping you from making the tests work? |
I was just avoiding a lot of changes as part of this pr only as those could be commonly used by other API spec tests as well when they are added and was trying to unblock the tiering API specs(this pr) because of that. However, I understand that we should not ship the code without coverage for it. |
@neetikasinghal want to finish this? |
Description
This PR introduces the API specification to tier the indices from hot to warm tier. This is currently going to be an experimental API.
Issues Resolved
Related PR on the opensearch side: opensearch-project/OpenSearch#13980
Related proposal: opensearch-project/OpenSearch#13294
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.