Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

neetikasinghal
Copy link
Contributor

@neetikasinghal neetikasinghal commented Jun 28, 2024

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.

@neetikasinghal
Copy link
Contributor Author

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

Copy link
Member

@dblock dblock left a 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.

spec/namespaces/indices.yaml Outdated Show resolved Hide resolved
spec/namespaces/indices.yaml Outdated Show resolved Hide resolved
@neetikasinghal
Copy link
Contributor Author

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?

@dblock
Copy link
Member

dblock commented Jun 28, 2024

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.

Copy link

github-actions bot commented Jul 30, 2024

Changes Analysis

Commit SHA: f759a99
Comparing To SHA: c7dbaaf

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

@neetikasinghal
Copy link
Contributor Author

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

Copy link
Member

@dblock dblock left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
spec/namespaces/indices.yaml Show resolved Hide resolved
spec/namespaces/indices.yaml Show resolved Hide resolved
spec/namespaces/indices.yaml Show resolved Hide resolved
spec/namespaces/indices.yaml Outdated Show resolved Hide resolved
tests/indices/index.yaml Outdated Show resolved Hide resolved
@neetikasinghal
Copy link
Contributor Author

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

Copy link
Member

@dblock dblock left a 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.

spec/namespaces/indices.yaml Show resolved Hide resolved
spec/schemas/tier.yaml Outdated Show resolved Hide resolved
spec/schemas/tier.yaml Outdated Show resolved Hide resolved
tests/indices/tier/warm.yaml Outdated Show resolved Hide resolved
tests/indices/tier/warm.yaml Outdated Show resolved Hide resolved
tests/indices/tier/warm.yaml Outdated Show resolved Hide resolved
tests/indices/tier/warm.yaml Outdated Show resolved Hide resolved
@neetikasinghal
Copy link
Contributor Author

neetikasinghal commented Aug 1, 2024

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(#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
Copy link
Member

dblock commented Aug 1, 2024

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

@neetikasinghal
Copy link
Contributor Author

@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: opensearch-project/OpenSearch#15076

@dblock
Copy link
Member

dblock commented Aug 6, 2024

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

@neetikasinghal
Copy link
Contributor Author

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

@dblock
Copy link
Member

dblock commented Aug 6, 2024

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

@neetikasinghal
Copy link
Contributor Author

@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
Copy link
Member

dblock commented Aug 6, 2024

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

#472

@neetikasinghal
Copy link
Contributor Author

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

@dblock
Copy link
Member

dblock commented Aug 7, 2024

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.

@neetikasinghal
Copy link
Contributor Author

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?

@dblock
Copy link
Member

dblock commented Aug 8, 2024

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

@neetikasinghal
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants