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 spec for new rename_alias_pattern and rename_alias_replacement parameters to snapshot restore #615

Merged
merged 10 commits into from
Nov 23, 2024

Conversation

mispencer
Copy link
Contributor

@mispencer mispencer commented Oct 16, 2024

Description

Add spec for new rename_alias_pattern and rename_alias_replacement parameters to snapshot restore implemented in Opensearch PR 16292

Issues Resolved

No related issues

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.

@mispencer
Copy link
Contributor Author

The implementing PR for this spec change is not yet merged.

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.

Looking good, needs a CHANGELOG, passing tests, etc.

- path: /stories
method: DELETE
status: [200, 404]
- path: /new_stories
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should call it something meaningful, maybe tales?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean new_stories? This is the restored-with-new-name index. I changed it to renamed_stories.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't love names like new_xyz or xyz_1 because we plan to use these samples to generate documentation. It's better with names like "movies" or "films", so instead of "new_stories" maybe we can use something semantically interesting like "tales"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is for the rename parameters for this API. An entirely new name like that would not fit the most common use case of those parameters, which is to restore a copy of existing index with a new, non-conflicting name. So, even for documentation, I think a prefixed name like currently is more suitable. The current documentation of restore supports this - it has an example of a prefixed name, recovered-logs, and a suffixed name, opendistro-reports-definitions_restored, but no example of a entirely new name for renamed indexes.

Copy link
Member

@dblock dblock Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, thanks. Maybe xyz-restored would make more semantic sense?

Take this opportunity to look at these restore tests and see if better names than "my-xyz" can be used. Think of what you'd want to read in the docs. NBD, but much appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have renamed this index and rewrote the rename parameters to match realistic use cases.

Copy link
Contributor

github-actions bot commented Oct 16, 2024

Changes Analysis

Commit SHA: ab1bec8
Comparing To SHA: 22bdd5c

API Changes

Summary

└─┬Paths
  ├─┬/_snapshot/{repository}/{snapshot}
  │ ├─┬PUT
  │ │ └─┬Requestbody
  │ │   └─┬application/json
  │ │     └─┬Schema
  │ │       └─┬ignore_unavailable
  │ │         └──[🔀] description (27563:30)
  │ └─┬POST
  │   └─┬Requestbody
  │     └─┬application/json
  │       └─┬Schema
  │         └─┬ignore_unavailable
  │           └──[🔀] description (27563:30)
  └─┬/_snapshot/{repository}/{snapshot}/_restore
    └─┬POST
      └─┬Requestbody
        └─┬application/json
          └─┬Schema
            ├──[➕] properties (27642:15)
            ├──[➕] properties (27649:15)
            ├──[➕] properties (27653:15)
            ├──[➕] properties (27657:15)
            ├─┬partial
            │ └──[➕] description (27626:30)
            ├─┬rename_pattern
            │ └──[➕] description (27633:30)
            ├─┬rename_replacement
            │ └──[➕] description (27641:30)
            ├─┬ignore_index_settings
            │ └──[➕] description (27605:30)
            ├─┬ignore_unavailable
            │ └──[➕] description (27611:30)
            ├─┬include_aliases
            │ └──[➕] description (27614:30)
            └─┬include_global_state
              └──[➕] description (27617:30)

Document Element Total Changes Breaking Changes
paths 13 0
  • Total Changes: 13
  • Modifications: 2
  • Additions: 11

Report

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

API Coverage

Before After Δ
Covered (%) 599 (58.67 %) 599 (58.67 %) 0 (0 %)
Uncovered (%) 422 (41.33 %) 422 (41.33 %) 0 (0 %)
Unknown 42 42 0

@mispencer mispencer force-pushed the AliasRenameSnapshotRestore branch from eadc2f8 to b18145d Compare October 16, 2024 21:00
@mispencer mispencer force-pushed the AliasRenameSnapshotRestore branch from fccd6a5 to 5bedafa Compare October 24, 2024 18:12
@mispencer mispencer marked this pull request as ready for review October 24, 2024 18:22
@mispencer
Copy link
Contributor Author

This feature has been merged.

@dblock
Copy link
Member

dblock commented Oct 24, 2024

This feature has been merged.

You'll need to update the 2.18 and 3.0 SHAs in CI as well for something that has the feature. I added some documentation for this in #642.

@dblock
Copy link
Member

dblock commented Nov 7, 2024

Now that 2.18 has been released this should be easier. Want to finish this @mispencer?

@mispencer mispencer force-pushed the AliasRenameSnapshotRestore branch from f5ccefb to 0050767 Compare November 17, 2024 18:18
@mispencer
Copy link
Contributor Author

Now that 2.18 has been released this should be easier. Want to finish this @mispencer?

Looks like this has already been updated in the main branch.

count: 3
- synopsis: Restore snapshot with rename_pattern and rename_replacement.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a future PR (unless this one needs more iterating) you should break up this test into snapshot/restore/index.yaml for basic scenarios restore/rename.yaml, etc., this way each stands alone as an example that can be reused in the appropriate documentation.

Copy link
Contributor

github-actions bot commented Nov 18, 2024

Spec Test Coverage Analysis

Total Tested
528 372 (70.45 %)

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.

Sorry to be a pest about CHANGELOG.

Style checker failed too. Doc uses indexes :(

CHANGELOG.md Outdated Show resolved Hide resolved
…rameters to snapshot restore

Signed-off-by: Spencer G. Jones <spencer.jones2@tylertech.com>
Signed-off-by: Spencer G. Jones <spencer.jones2@tylertech.com>
Signed-off-by: Spencer G. Jones <spencer.jones2@tylertech.com>
Signed-off-by: Spencer G. Jones <spencer.jones2@tylertech.com>
Signed-off-by: Spencer G. Jones <spencer.jones2@tylertech.com>
Signed-off-by: Spencer G. Jones <spencer.jones2@tylertech.com>
…Add missing parameters.

Signed-off-by: Spencer G. Jones <spencer.jones2@tylertech.com>
Signed-off-by: Spencer G. Jones <spencer.jones2@tylertech.com>
Signed-off-by: Spencer G. Jones <spencer.jones2@tylertech.com>
@mispencer mispencer force-pushed the AliasRenameSnapshotRestore branch from 0ba119d to 4d463bb Compare November 22, 2024 22:20
Signed-off-by: Spencer G. Jones <spencer.jones2@tylertech.com>
@mispencer mispencer force-pushed the AliasRenameSnapshotRestore branch from 4d463bb to ab1bec8 Compare November 22, 2024 22:24
@dblock dblock merged commit 8cb29a4 into opensearch-project:main Nov 23, 2024
27 checks passed
@dblock
Copy link
Member

dblock commented Nov 23, 2024

Merged.

Would you have a bit of time to break up the long test into snapshot/restore/index.yaml for basic scenarios restore/rename.yaml, etc.? This way each stands alone as an example that can be reused in the appropriate documentation.

@@ -294,7 +294,7 @@ components:
type: object
properties:
ignore_unavailable:
description: If `true`, the request ignores data streams and indexes in `indices` that are missing or closed. If `false`, the request returns an error for any data stream or index that is missing or closed.
description: If `true`, the request ignores data streams and indexes in `indexes` that are missing or closed. If `false`, the request returns an error for any data stream or index that is missing or closed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this refers to a variable (indices) then quoting it will work, so I think this means indices.

@mispencer mispencer deleted the AliasRenameSnapshotRestore branch December 11, 2024 17:30
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.

2 participants