opensearch-api-specification icon indicating copy to clipboard operation
opensearch-api-specification copied to clipboard

Add api spec for tiering of indices from hot to warm

Open neetikasinghal opened this issue 1 year ago • 20 comments

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.

neetikasinghal avatar Jun 28 '24 18:06 neetikasinghal

@dblock please review, let me know if there is anything else that needs to be added.

neetikasinghal avatar Jun 28 '24 18:06 neetikasinghal

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?

neetikasinghal avatar Jun 28 '24 19:06 neetikasinghal

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.

dblock avatar Jun 28 '24 20:06 dblock

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

github-actions[bot] avatar Jul 30 '24 21:07 github-actions[bot]

@dblock @nhtruong do u mind taking another look at the pr?

neetikasinghal avatar Jul 30 '24 22:07 neetikasinghal

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

neetikasinghal avatar Aug 01 '24 00:08 neetikasinghal

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?

neetikasinghal avatar Aug 01 '24 19:08 neetikasinghal

@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 avatar Aug 01 '24 19:08 dblock

@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 avatar Aug 02 '24 01:08 neetikasinghal

@neetikasinghal Looks like the OpenSearch PR got merged, want to finish this and add a 2.17 to CI?

dblock avatar Aug 06 '24 17:08 dblock

@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 avatar Aug 06 '24 18:08 neetikasinghal

@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 avatar Aug 06 '24 18:08 dblock

@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.

neetikasinghal avatar Aug 06 '24 20:08 neetikasinghal

@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 avatar Aug 06 '24 20:08 dblock

@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 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?

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.)

neetikasinghal avatar Aug 07 '24 18:08 neetikasinghal

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.

dblock avatar Aug 07 '24 19:08 dblock

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 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.

@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?

neetikasinghal avatar Aug 08 '24 17:08 neetikasinghal

@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 avatar Aug 08 '24 18:08 dblock

@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 avatar Aug 09 '24 17:08 neetikasinghal

@neetikasinghal want to finish this?

dblock avatar Nov 07 '24 14:11 dblock