-
Notifications
You must be signed in to change notification settings - Fork 27
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
Updated RFS to filter out system indices by default #763
Conversation
Signed-off-by: Chris Helma <chelma+github@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #763 +/- ##
============================================
- Coverage 68.50% 68.41% -0.09%
+ Complexity 1583 1579 -4
============================================
Files 270 273 +3
Lines 11175 11361 +186
Branches 736 734 -2
============================================
+ Hits 7655 7773 +118
- Misses 3118 3187 +69
+ Partials 402 401 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if (indexAllowlist.isEmpty()) { | ||
accepted = !index.getName().startsWith("."); | ||
} else { | ||
accepted = indexAllowlist.contains(index.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make a backlog item for this to support regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done: https://opensearch.atlassian.net/browse/MIGRATIONS-1804
}; | ||
repoDataProvider.getIndicesInSnapshot(snapshotName).stream() | ||
.filter(FilterScheme.filterIndicesByAllowList(indexAllowlist, logger)) | ||
.peek(index -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we should be able to do a .forEach
instead of the peek and count
}; | ||
repoDataProvider.getIndicesInSnapshot(snapshotName).stream() | ||
.filter(FilterScheme.filterIndicesByAllowList(indexAllowlist, logger)) | ||
.peek(index -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same as above with foreach
if (indexAllowlist.isEmpty()) { | ||
accepted = !index.getName().startsWith("."); | ||
} else { | ||
accepted = indexAllowlist.contains(index.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
() -> log.info("Index " + index.getName() + " already existed; no work required") | ||
); | ||
}) | ||
.count(); // Force the stream to execute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
foreach() will force the stream to run. I think that this will generate a linting error as it is since the output of count() is discarded.
Signed-off-by: Chris Helma <25470211+chelma@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add test coverage for this filtering logic.
Covered in follow-up task (see: https://opensearch.atlassian.net/browse/MIGRATIONS-1804)
Description
.
) by default. The user can still set an explicit allowlist to bypass this default behaviorIssues Resolved
Bug fix
Testing
Tested manually on my laptop. Spun up the docker compose setup for RFS and manually added a new index called
.test_should_not_migrate
:I then took a snapshot and ran the metadata migration code, which skipped that that index:
And ran the document migration code, which also skipped it:
Check List
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.