Skip to content

Add target_index_settings to rollup specification #822

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

Merged

Conversation

MrChaos1993
Copy link
Contributor

Description

Allow to create index with specified settings during rollup

Issues Resolved

opensearch-project/index-management#1376

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

dblock commented Feb 20, 2025

@MrChaos1993 amend your commit with -s to pass DCO

Exercise an API in a test that returns this data, see https://github.com/opensearch-project/opensearch-api-specification/blob/main/TESTING_GUIDE.md

Copy link
Contributor

github-actions bot commented Feb 25, 2025

Changes Analysis

Commit SHA: f9d87fd
Comparing To SHA: a8ea65d

API Changes

Summary

└─┬Components
  └─┬rollups._common___Rollup
    └──[➕] properties (65465:9)

Document Element Total Changes Breaking Changes
components 1 0
  • Total Changes: 1
  • Additions: 1

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/15781582685/artifacts/3375164786

API Coverage

Before After Δ
Covered (%) 666 (65.23 %) 666 (65.23 %) 0 (0 %)
Uncovered (%) 355 (34.77 %) 355 (34.77 %) 0 (0 %)
Unknown 91 91 0

@nhtruong
Copy link
Collaborator

Test suites that involve the new test failed with Expected status 201, but received 400: application/json. Invalid field [target_index_settings] found in Rollup.

Don't forget to add a line in the CHANGELOG.md describing the change from this PR.

@MrChaos1993 MrChaos1993 force-pushed the feat/rollup-index-settings branch 2 times, most recently from 9def519 to 9fa1c39 Compare February 28, 2025 13:49
@MrChaos1993
Copy link
Contributor Author

Hi @nhtruong!
Could you please review this again?

@nhtruong
Copy link
Collaborator

nhtruong commented Mar 3, 2025

We still get this error Invalid field [target_index_settings] found in Rollup. in the test suite against OS 3.0.0
Was target_index_settings added to 3.0.0? Or was it added to a later/unreleased version?

@MrChaos1993
Copy link
Contributor Author

It was merged to main branch (Link to PR) and require version 3.0.0 or above.
I am not sure is it released or not, could you please help me?

@nhtruong
Copy link
Collaborator

nhtruong commented Mar 3, 2025

@MrChaos1993 That looks like it belongs to an unreleased version of the index-management plugin.

@dblock correct me if I'm wrong but we only have the capability to test against unreleased OpenSearch right now. And It's too complicated to setup a cluster with unreleased plugins. So, in that case, we should this PR on hold for now?

@dblock
Copy link
Member

dblock commented Mar 4, 2025

@dblock correct me if I'm wrong but we only have the capability to test against unreleased OpenSearch right now. And It's too complicated to setup a cluster with unreleased plugins. So, in that case, we should this PR on hold for now?

You could use an unreleased version of the plugin too by installing the specific build in the container, but unsure it's worth it.

@nhtruong nhtruong added the blocked (release pending) Pending on the release of an OS or plugin version label Mar 4, 2025
@nhtruong
Copy link
Collaborator

nhtruong commented Mar 4, 2025

@dblock correct me if I'm wrong but we only have the capability to test against unreleased OpenSearch right now. And It's too complicated to setup a cluster with unreleased plugins. So, in that case, we should this PR on hold for now?

You could use an unreleased version of the plugin too by installing the specific build in the container, but unsure it's worth it.

I don't think it's worth it to set it up per PR like this, just to remove it once the new version of the plugin is released. Let's wait, @MrChaos1993. Sorry we don't have a more elegant solution for this right now. I've created a new label for this type of PRs.

@MrChaos1993
Copy link
Contributor Author

@dblock correct me if I'm wrong but we only have the capability to test against unreleased OpenSearch right now. And It's too complicated to setup a cluster with unreleased plugins. So, in that case, we should this PR on hold for now?

You could use an unreleased version of the plugin too by installing the specific build in the container, but unsure it's worth it.

I don't think it's worth it to set it up per PR like this, just to remove it once the new version of the plugin is released. Let's wait, @MrChaos1993. Sorry we don't have a more elegant solution for this right now. I've created a new label for this type of PRs.

Sure, no problem!
I just wondering about missing specification for the new field, so If it's better to wait until release a newer version of plugin I totally agree.
Only one question should I tag someone when a newer version of the plugin will be released, or you will merge\restart check on your own?

@nhtruong
Copy link
Collaborator

nhtruong commented Mar 10, 2025

Sure, no problem! I just wondering about missing specification for the new field, so If it's better to wait until release a newer version of plugin I totally agree. Only one question should I tag someone when a newer version of the plugin will be released, or you will merge\restart check on your own?

We'll need to update this PR to point the test with the new params to the release that can accept that params. We will check back in a couple of month. But if you notice that it's been released sooner than that, feel free to leave a message here :) Thank you so much for helping us with updating this endpoint.

@MrChaos1993 MrChaos1993 force-pushed the feat/rollup-index-settings branch from 9fa1c39 to f337c1b Compare May 27, 2025 05:38
@MrChaos1993 MrChaos1993 force-pushed the feat/rollup-index-settings branch 2 times, most recently from e8a623f to 52e2e67 Compare June 3, 2025 11:56
@MrChaos1993
Copy link
Contributor Author

@nhtruong, Hi! I see that version 3.0.0 has been released and this pull request can be merged :)

nhtruong
nhtruong previously approved these changes Jun 4, 2025
nhtruong
nhtruong previously approved these changes Jun 4, 2025
dblock
dblock previously requested changes Jun 7, 2025
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.

Iterate till tests pass?

@MrChaos1993
Copy link
Contributor Author

As I see, tests are failing due to reasons unrelated to this PR. Please check the build logs for more details.

@dblock
Copy link
Member

dblock commented Jun 8, 2025

@MrChaos1993 We'll need some help.

  1. Rebase, these issues may already be fixed.
  2. If it fails again, look at the errors, if the error is not related to this PR make sure there's an issue about it, if not open one - for example if a test is failing in this PR but succeeds on main, open something that says "XYZ test is flaky".
  3. Comment on this issue with all the unrelated failure issues.
  4. I can kick off a retry (doing this now for the failures here), or you can force it with a rebase.

Signed-off-by: Aleksandr Tuliakov <[email protected]>
Signed-off-by: Aleksandr Tuliakov <[email protected]>
Signed-off-by: Aleksandr Tuliakov <[email protected]>
Use different id for rollup job with index settings

Signed-off-by: Aleksandr Tuliakov <[email protected]>
Copy link
Contributor

Spec Test Coverage Analysis

Total Tested
644 642 (99.69 %)

@Xtansia Xtansia dismissed dblock’s stale review June 22, 2025 21:59

CI is green

@Xtansia Xtansia merged commit bcd557d into opensearch-project:main Jun 22, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked (release pending) Pending on the release of an OS or plugin version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants