Retain local indices on exporter type change

[dzane17]

Description

• Remove logic that deletes all Query Insights indices when exporter type setting is changed
• Run daily deletion job regardless of current exporter
• Introduced LocalIndexLifecycleManager to hold all delete-after state and logic

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.

[ansjcy]

Do you think we should add an API endpoint to do this?

[dzane17]

No, admins can delete indices manually I believe and retention policy will still apply

[ansjcy]

makes sense.

[ansjcy]

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.

[dzane17]

That is a good point. I added the ability to immediately delete all local indices by setting delete_after_days = 0. Previously the min value was 1. 0 means no retention period so I think it makes sense to use as a "delete now" option.

[ansjcy]

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.

[ansjcy]

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.

[ansjcy]

I have some deeper concerns on the retention policy behavior after this change.

1. If we disable local index exporter, that delete_after job will not delete the expired indices now because the related logic will only run if local index exporter is enabled

   query-insights/src/main/java/org/opensearch/plugin/insights/core/service/QueryInsightsService.java

   Line 679 in 34391ac

   if (topQueriesExporter != null && topQueriesExporter.getClass() == LocalIndexExporter.class) {

   , these indices will become orphaned forever.
2. If "enable local index" -> "disable all features" , because of this check

   query-insights/src/main/java/org/opensearch/plugin/insights/core/service/QueryInsightsService.java

   Line 604 in 34391ac

   if (isAnyFeatureEnabled()) {

   , the old indices will also remain there forever.

Let's think about even more scenarios: like

• local_index -> disable all features -> reenable with local index
• local_index -> disable all -> reenable with none
  ...

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.

[codecov]

Codecov Report

❌ Patch coverage is 78.48101% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.85%. Comparing base (612912d) to head (3b178ca).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ights/core/service/LocalIndexLifecycleManager.java	83.60%	10 Missing ⚠️
...in/insights/core/service/QueryInsightsService.java	61.11%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #465      +/-   ##
============================================
+ Coverage     78.60%   78.85%   +0.25%     
- Complexity      620      625       +5     
============================================
  Files            52       53       +1     
  Lines          2346     2341       -5     
  Branches        228      226       -2     
============================================
+ Hits           1844     1846       +2     
+ Misses          381      378       -3     
+ Partials        121      117       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dzane17]

I decoupled the logic between the daily deletion job and the active exporter. The daily job now runs every day, regardless of which exporter is enabled. Previously, deleteAfter was stored in LocalIndexExporter, which only existed when the local index exporter was enabled. I introduced a new LocalIndexLifecycleManager class to hold all delete-after–related state and logic.