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

[CCI] Create index lifecycle guide #362

Merged
merged 6 commits into from
Apr 24, 2023

Conversation

Nicksqain
Copy link
Contributor

@Nicksqain Nicksqain commented Apr 8, 2023

Description

  • Create index lifecycle guide

Issues Resolved

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.

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
@VachaShah
Copy link
Collaborator

Thank you for contributing @Nicksqain! Can you add a Changelog entry for this? Also, I see you have 3 PRs for adding guides, you can also combine them into one with a single Changelog entry if that works.

@Nicksqain
Copy link
Contributor Author

@VachaShah Hello! thanks for the feedback! I will definitely add an entry. If I add to one, will the others have a "skip changelog"?

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2023

Codecov Report

Merging #362 (a712305) into main (5fa4e8c) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main     #362   +/-   ##
=======================================
  Coverage   72.01%   72.01%           
=======================================
  Files          77       77           
  Lines        7190     7190           
=======================================
  Hits         5178     5178           
  Misses       2012     2012           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@VachaShah
Copy link
Collaborator

@VachaShah Hello! thanks for the feedback! I will definitely add an entry. If I add to one, will the others have a "skip changelog"?

If you keep the 3 separate PRs, then you can add Changelog entry in each PR. Or you can combine all 3 PRs into 1 and add a Changelog entry.

For Changelog, you can add an entry in the Added section for Unreleased.

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
@Nicksqain
Copy link
Contributor Author

Thank you for contributing @Nicksqain! Can you add a Changelog entry for this? Also, I see you have 3 PRs for adding guides, you can also combine them into one with a single Changelog entry if that works.

Can we wait for the index lifecycle to merge first and then re-run the link checker?
I suggest simply doing a merge in the order of the tasks in #355 and adding the changelog entry one by one. What do you think?

Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

Thank you @Nicksqain! I tried the code in the guide and had a few comments.


```python
print(client.indices.exists(index='burner')) # => False
client.create(index='burner', body={'lorem': 'ipsum'})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be client.index with id parameter:

client.index(id='1', index='burner', body={'lorem': 'ipsum'})

We can also delete multiple indices at once:

```python
client.indices.delete(index=['movies', 'paintings', 'burner'], ignore=[404])
Copy link
Collaborator

Choose a reason for hiding this comment

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

ignore is not an option for python. So this would be:

print(client.indices.delete(index=['paintings', 'burner']))

since movies is deleted in the previous line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But other guides for clients have the presence of ignoring. This is part of the guide.
Did I make a mistake or is it just that there is no option to ignore the 404 error?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the code, the parameter available for ignoring non-existing indices is ignore_unavailable. I tried it and the following works:

print(client.indices.delete(index=['art', 'paintings', 'burner'], ignore_unavailable=True))

where art index does not exist. The response is:

{'acknowledged': True}

Lets change it to ignore_unavailable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks for the correction! I had my doubts, as the rust client also had this option.

client.indices.delete(index=['movies', 'paintings', 'burner'], ignore=[404])
```

Notice that we are passing `ignore: 404` to the request. This tells the client to ignore the `404` error if the index doesn't exist for deletion. Without it, the above `delete` request will throw an error because the `movies` index has already been deleted in the previous example.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This section is not applicable since ignore is not an option for this client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in the repository I found an application for ignoring the lack of an index.
(It takes an array of)
Still need to delete the section?
изображение

Copy link
Collaborator

Choose a reason for hiding this comment

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

ignore_unavailable seems to be the correct option. See

:arg ignore_unavailable: Ignore unavailable indexes (default:

@VachaShah
Copy link
Collaborator

Thank you for contributing @Nicksqain! Can you add a Changelog entry for this? Also, I see you have 3 PRs for adding guides, you can also combine them into one with a single Changelog entry if that works.

Can we wait for the index lifecycle to merge first and then re-run the link checker? I suggest simply doing a merge in the order of the tasks in #355 and adding the changelog entry one by one. What do you think?

Sure, we can merge this PR first and you can fix the changelogs one by one for your PRs after that. I just added a few comments for this PR.

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
@Nicksqain Nicksqain requested a review from VachaShah April 20, 2023 18:58
@Nicksqain
Copy link
Contributor Author

@VachaShah I've added changes! Happy to help!
Looking forward to the following preferences :)

Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

This looks good! Can you pull the latest changes from main and resolve the conflicts in the Changelog? We should be good to merge then!

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
@Nicksqain Nicksqain requested a review from VachaShah April 21, 2023 19:01
@Nicksqain
Copy link
Contributor Author

@VachaShah
Updated the branch!
I accidentally saved the breaks of my linter. Apologies for not rolling back and doing a force push

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
@Nicksqain Nicksqain requested a review from VachaShah April 24, 2023 17:26
@VachaShah VachaShah merged commit 12b2748 into opensearch-project:main Apr 24, 2023
roma2023 pushed a commit to roma2023/opensearch-py that referenced this pull request May 9, 2023
* Create index lifecycle guide

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

* Add changelog entry

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

* Replace ignore param and add short corrections

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

* reset spaces

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

* Correct indents

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

---------

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Raman Saparkhan <romasaparhan19@gmail.com>
roma2023 pushed a commit to roma2023/opensearch-py that referenced this pull request Jul 4, 2023
* Create index lifecycle guide

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

* Add changelog entry

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

* Replace ignore param and add short corrections

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

* reset spaces

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

* Correct indents

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>

---------

Signed-off-by: Alexei Karikov <karikov.alist.ru@gmail.com>
Signed-off-by: Raman Saparkhan <romasaparhan19@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Guide] Index Lifecycle
4 participants