-
Notifications
You must be signed in to change notification settings - Fork 113
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
BWC for rollover skip, restricted index pattern #371
BWC for rollover skip, restricted index pattern #371
Conversation
Signed-off-by: bowenlan-amzn <[email protected]>
b979f0b
to
99b4694
Compare
Codecov Report
@@ Coverage Diff @@
## main #371 +/- ##
============================================
+ Coverage 74.88% 75.38% +0.49%
- Complexity 2138 2159 +21
============================================
Files 262 263 +1
Lines 12461 12429 -32
Branches 1969 1975 +6
============================================
+ Hits 9331 9369 +38
+ Misses 2060 1974 -86
- Partials 1070 1086 +16
Continue to review full report at Codecov.
|
Signed-off-by: bowenlan-amzn <[email protected]>
if (this.settings.get(ManagedIndexSettings.ROLLOVER_SKIP.key).isNullOrBlank()) { | ||
return this.settings.getAsBoolean(LegacyOpenDistroManagedIndexSettings.ROLLOVER_SKIP.key, false) | ||
} |
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.
I believe this is unnecessary because the fallback setting for ManagedIndexSettings.ROLLOVER_SKIP is set to LegacyOpenDistroManagedIndexSettings.ROLLOVER_SKIP when initialized, so the open distro setting would be checked anyways
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.
Based on above getRolloverAlias
, probably this is not working as that. Write like this should be safe.
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.
Thanks, I looked into it deeper and it seems that the fallback settings do not operate as I thought for index settings.
val RESTRICTED_INDEX_PATTERN = Setting.simpleString( | ||
"opendistro.index_state_management.restricted_index_pattern", | ||
ManagedIndexSettings.DEFAULT_RESTRICTED_PATTERN, | ||
Setting.Property.NodeScope, | ||
Setting.Property.Dynamic, | ||
Setting.Property.Deprecated | ||
) |
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.
Thanks for adding this too Bowen
* BWC for rollover skip, restricted index pattern Signed-off-by: bowenlan-amzn <[email protected]> * Remove VisibleForTesting Signed-off-by: bowenlan-amzn <[email protected]>
Signed-off-by: bowenlan-amzn [email protected]
Description of changes:
Though these 2 settings are released after OpenSearch, we backport them to AWS Elasticsearch code base, to keep the minimal code difference, we want to add the change for supporting setting BWC between Elasticsearch and OpenSearch code bases.
CheckList:
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.