fix: Separate internal ignore settings from user ignore settings in restore#20494
fix: Separate internal ignore settings from user ignore settings in restore#20494piyushk6130 wants to merge 2 commits intoopensearch-project:mainfrom
Conversation
…estore Internal ignore settings should override user-unremovable settings protection to allow proper filtering of settings like remote_store.* during cross-cluster restores. Signed-off-by: Piyush Kumar <piykumab@amazon.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThe change introduces separate internal and user-facing ignore settings lists in RestoreService. Internal ignore patterns are collected separately and applied first in the filtering logic, while user-protected settings protection semantics are preserved. No public API modifications. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
❌ Gradle check result for 1dfa515: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
|
||
| Predicate<String> settingsFilter = k -> { | ||
| // Check internal ignore patterns first (they override protection) | ||
| for (String filterKey : internalKeyFilters) { |
There was a problem hiding this comment.
can we replace the for loop with a exists check in the Set
| if (k.equals(filterKey)) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
we need to cover this change in some UT or IT
Add tests to verify that internal ignore patterns can filter protected settings while user ignore patterns respect protection. Also optimize the predicate to use Set.contains() instead of iteration. Signed-off-by: Piyush Kumar <piykumab@amazon.com>
piyushk6130
force-pushed
the
fix-internal-ignore-settings-restore
branch
from
c55065f to
377242f
Compare
|
❌ Gradle check result for 377242f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
This PR is stalled because it has been open for 30 days with no activity. |
3 participants
Description
This PR fixes an issue where internal ignore settings were being blocked by user-unremovable settings protection during snapshot restore operations.
Problem
When restoring snapshots across clusters (e.g., from a remote-store enabled cluster to a non-remote-store cluster), internal settings like index.remote_store.* need to be filtered out. However, the current implementation combines internal ignore settings with user ignore settings, causing internal settings to be blocked by the USER_UNREMOVABLE_SETTINGS protection check.
Solution
Separate internal ignore settings into their own filter lists (internalKeyFilters and internalSimpleMatchPatterns) that are evaluated first and bypass the user-unremovable settings protection. This ensures:
Internal ignore settings (like index.remote_store.*) are always filtered regardless of protection status
User ignore settings continue to respect the USER_UNREMOVABLE_SETTINGS protection
Testing
Manually tested cross-cluster restore scenarios:
Remote store → Non-remote store: Remote store settings correctly stripped ✓
Non-remote store → Remote store: Settings correctly applied by target cluster ✓
Remote store → Remote store: Settings preserved ✓
Non-remote store → Non-remote store: No changes ✓