-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Remote Store] Add support to create index with remote store by default #6932
[Remote Store] Add support to create index with remote store by default #6932
Conversation
Gradle Check (Jenkins) Run Completed with:
|
41b9ae8
to
065c8aa
Compare
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #6932 +/- ##
============================================
- Coverage 71.10% 70.79% -0.31%
+ Complexity 59808 59514 -294
============================================
Files 4852 4852
Lines 285138 285176 +38
Branches 41107 41118 +11
============================================
- Hits 202755 201902 -853
- Misses 66040 66679 +639
- Partials 16343 16595 +252
... and 472 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Gradle Check (Jenkins) Run Completed with:
|
68667e1
to
e8b08d4
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Varun Bansal <bansvaru@amazon.com>
790c601
to
4fef117
Compare
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Varun Bansal <bansvaru@amazon.com>
Signed-off-by: Varun Bansal <bansvaru@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
# Controls whether cluster imposes index creation only with remote store enabled | ||
# cluster.remote_store.enabled.force: true |
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.
add one setting for remote translog as well .
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.
Sure, updated
&& !INDEX_REMOTE_STORE_ENABLED_SETTING.get(requestSettings))) { | ||
return; | ||
} | ||
String segmentStoreRepo = requestSettings.get(SETTING_REMOTE_STORE_REPOSITORY); |
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 : Lets call it remoteStoreRepo
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.
updated
if (FeatureFlags.isEnabled(FeatureFlags.REMOTE_STORE) | ||
&& FeatureFlags.isEnabled(FeatureFlags.REPLICATION_TYPE) |
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.
We need to move these settings under FEATURE_FLAGGED_CLUSTER_SETTINGS
. In that case, we won't need to put these checks here and user won't be able be set it w/o enabling the feature flags first.
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 pointing this out. i'll update
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.
in our case its two features we are dependent on. FEATURE_FLAGGED_CLUSTER_SETTINGS only supports adding settings which are dependent on single feature
.put(SETTING_REMOTE_STORE_ENABLED, true) | ||
.put(SETTING_REMOTE_STORE_REPOSITORY, segmentStoreRepo); | ||
|
||
if (!Objects.equals(requestSettings.get(INDEX_REMOTE_TRANSLOG_STORE_ENABLED_SETTING.getKey()), "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.
we also need to check if translog_repo
is also supplied . The reason being either we pick both enabled
and repo
from index settings . If not provided , fall back to cluster level settings.
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.
do we not allow user to just send INDEX_REMOTE_TRANSLOG_STORE_ENABLED_SETTING
as true and pick translog repo from cluster settings? I think we should allow this, let me know what you think
Signed-off-by: Varun Bansal <bansvaru@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Varun Bansal <bansvaru@amazon.com>
…riding settings Signed-off-by: Varun Bansal <bansvaru@amazon.com>
b7e50c9
to
3151753
Compare
Gradle Check (Jenkins) Run Completed with:
|
private static void updateRemoteStoreSettings(Settings.Builder settingsBuilder, Settings requestSettings, Settings clusterSettings) { | ||
if (CLUSTER_REMOTE_STORE_ENABLED_SETTING.get(clusterSettings)) { | ||
// Verify if we can create a remote store based index based on user provided settings | ||
if (!canCreateRemoteStoreIndex(requestSettings)) { |
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: canCreateRemoteStoreIndex(requestSettings) == false
(Applicable to all the places where we are using !
in if condition)
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.
Sure
return; | ||
} | ||
|
||
settingsBuilder.put(SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT).put(SETTING_REMOTE_STORE_ENABLED, true); |
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.
Why are we adding Segment Replication setting explicitly here? Are we sure that replication type will always be segment at this point?
As I understand, Segment replication also has a cluster level setting. settingsBuilder
should be updated by cluster and index level setting of segment replication prior to updateRemoteStoreSettings
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.
if there is an index level setting for segment replication then it would have been put by the user. We already have check for it.
For cluster level setting, if its set to SEGMENT, we dont have any problem. If its set to DOCUMENT then we are overriding and sending SEGMENT as replication.
I'll update to throw an error incase of cluster level settings being document
Signed-off-by: Varun Bansal <bansvaru@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
# cluster.remote_store.enabled: true | ||
# | ||
# Repository to use for segment upload while enforcing remote store for an index | ||
# cluster.remote_store.repository: my-repo-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.
will it throw exception if remote repo is not setup and prevent index creation?
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.
yes. we have checks during index creation
…lt (opensearch-project#6932) Signed-off-by: Varun Bansal <bansvaru@amazon.com>
Description
Allows clusters to be configured for all indices to be Remote Store backed by default. The user doesn't need to specify the settings for remote store and can create an index without specifying Remote Store settings. The cluster will automatically use the default settings and create the index with Remote Store enabled.
Issues Resolved
[List any issues this PR will resolve]
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.