opensearch-api-specification
opensearch-api-specification copied to clipboard
Add api spec for tiering of indices from hot to warm
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: https://github.com/opensearch-project/OpenSearch/pull/13980 Related proposal: https://github.com/opensearch-project/OpenSearch/issues/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.
@dblock please review, let me know if there is anything else that needs to be added.
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?
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 Analysis
Commit SHA: f759a99a36c389234cd93b17c1359bd8ce0edced Comparing To SHA: c7dbaafc5ab684b0274d9695c5209f3f61401dcc
API Changes
Summary
├─┬Paths
│ └──[➕] path (10189:3)
└─┬Components
├──[➕] responses (25123:7)
├──[➕] parameters (18452:7)
├──[➕] parameters (18427:7)
├──[➕] parameters (18416:7)
├──[➕] parameters (18444:7)
├──[➕] parameters (18437:7)
├──[➕] parameters (18461:7)
└──[➕] schemas (49245:7)
| Document Element | Total Changes | Breaking Changes |
|---|---|---|
| paths | 1 | 0 |
| components | 8 | 0 |
- Total Changes: 9
- Additions: 9
Report
The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10273263322/artifacts/1782771613
API Coverage
| Before | After | Δ | |
|---|---|---|---|
| Covered (%) | 510 (49.95 %) | 510 (49.95 %) | 0 (0 %) |
| Uncovered (%) | 511 (50.05 %) | 511 (50.05 %) | 0 (0 %) |
| Unknown | 24 | 25 | 1 |
@dblock @nhtruong do u mind taking another look at the pr?
Good progress, close! See below and get all tests to pass.
thanks @dblock for reviewing, successful run of tests is dependent on this pr: https://github.com/opensearch-project/opensearch-api-specification/pull/454
Good progress, close! See below and get all tests to pass.
thanks @dblock for reviewing, successful run of tests is dependent on this pr: #454
@dblock should we add a higher version on the tests as the feature flag pr(https://github.com/opensearch-project/opensearch-api-specification/pull/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?
@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?
That would be semantically incorrect. Let's sort these PRs one after another.
@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?
That would be semantically incorrect. Let's sort these PRs one after another.
@dblock raised a pr in opensearch core package to fix the bug: https://github.com/opensearch-project/OpenSearch/pull/15076
@neetikasinghal Looks like the OpenSearch PR got merged, want to finish this and add a 2.17 to CI?
@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? Also, the spell checker complains of the Dopensearch here(https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10205504557/job/28236397605?pr=368), any suggestions to avoid this?
@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.
Also, the spell checker complains of the Dopensearch here(https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10205504557/job/28236397605?pr=368), any suggestions to avoid this?
Just add it to .cspell.
@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.
@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 https://github.com/opensearch-project/opensearch-api-specification/pull/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 tests/, for example tests/features/experimental/tiered_remote_index in which we'd locate a docker-compose.yml and YAML tests just for that feature? We can add another GHA workflow that only deals with things in tests/features differently from the main tests that we have now?
https://github.com/opensearch-project/opensearch-api-specification/pull/472
@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
tests/, for exampletests/features/experimental/tiered_remote_indexin which we'd locate adocker-compose.ymland YAML tests just for that feature? We can add another GHA workflow that only deals with things intests/featuresdifferently from the main tests that we have now?
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.)
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.
I implemented it in https://github.com/opensearch-project/opensearch-api-specification/pull/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 default folder and delete the customer docker-compose.
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.)
AFAIK not.
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.
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
defaultfolder and delete the customer docker-compose.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.)
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?
@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?
@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?