Persist local indices on exporter type change #465
Conversation
Signed-off-by: David Zane <davizane@amazon.com>
dzane17
changed the title
| * @param client OpenSearch Client | ||
| * @param localIndexExporter the exporter to handle the local index operations | ||
| */ | ||
| void deleteAllTopNIndices(final Client client, final LocalIndexExporter localIndexExporter) { |
There was a problem hiding this comment.
Do you think we should add an API endpoint to do this?
There was a problem hiding this comment.
No, admins can delete indices manually I believe and retention policy will still apply
There was a problem hiding this comment.
I think one caveat here is, if there are issues with local index exporters, we won't have a quick solution to solve it. like, for the local index reader performance issue we encountered this year, we can simply disable this setting to remove all local index.
If any similar unknown issue happens later, we will need to delete user's indices manually (without the validation logic in our code), or wait for 1 day until the retention job kicks in.
That's the reason I feel it's better to have an API to delete all indices with correct validation logic would make sense. Another alternative is to lower the min value of the delete_after setting to say 1 hour. but the deletion logic will be more complicated since we will need to delete the documents instead of the indices.
|
Let's also create a documentation PR for this, we missed adding the expectation for disabling the local index (i.e. warning: local index will be deleted after you disable local index). We should call this out now in the doc: your index will not be deleted, if you need to delete, do it manually. |
| assertTrue(fieldTypeCacheStats.containsKey(MISS_COUNT)); | ||
| } | ||
|
|
||
| public void testDeleteAllTopNIndices() throws IOException { |
There was a problem hiding this comment.
instead of removing the tests, we should have tests to validate that the local index "will not be deleted" after disabling.
| * @param client OpenSearch Client | ||
| * @param localIndexExporter the exporter to handle the local index operations | ||
| */ | ||
| void deleteAllTopNIndices(final Client client, final LocalIndexExporter localIndexExporter) { |
There was a problem hiding this comment.
I think one caveat here is, if there are issues with local index exporters, we won't have a quick solution to solve it. like, for the local index reader performance issue we encountered this year, we can simply disable this setting to remove all local index.
If any similar unknown issue happens later, we will need to delete user's indices manually (without the validation logic in our code), or wait for 1 day until the retention job kicks in.
That's the reason I feel it's better to have an API to delete all indices with correct validation logic would make sense. Another alternative is to lower the min value of the delete_after setting to say 1 hour. but the deletion logic will be more complicated since we will need to delete the documents instead of the indices.
|
I have some deeper concerns on the retention policy behavior after this change.
Let's think about even more scenarios: like
IMO we should always enforce the retention job, regardless whether features are enabled, or what type of exporter is enabled. we can also consider adding "none" to delete after , so that people can also disable this retention job. |
2 participants
Description
Remove logic that deletes all Query Insights indices when exporter type setting is changed
Issues Resolved
Resolves #453
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.