Skip to content
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

Adds the replication type index setting, alongside a formal notion of feature flags #3037

Merged
merged 8 commits into from
Apr 25, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,17 @@ public Iterator<Setting<?>> settings() {
Property.IndexScope
);

public static final String SETTING_SEGMENT_REPLICATION = "index.replication.segment_replication";
/**
* Used to specify if the index should use segment replication. If false, document replication is used.
*/
public static final Setting<Boolean> INDEX_SEGMENT_REPLICATION_SETTING = Setting.boolSetting(
SETTING_SEGMENT_REPLICATION,
false,
Property.IndexScope,
Property.Final
);

public static final String SETTING_AUTO_EXPAND_REPLICAS = "index.auto_expand_replicas";
public static final Setting<AutoExpandReplicas> INDEX_AUTO_EXPAND_REPLICAS_SETTING = AutoExpandReplicas.SETTING;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.opensearch.cluster.routing.allocation.decider.ShardsLimitAllocationDecider;
import org.opensearch.common.logging.Loggers;
import org.opensearch.common.settings.Setting.Property;
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.index.IndexModule;
import org.opensearch.index.IndexSettings;
import org.opensearch.index.IndexSortConfig;
Expand Down Expand Up @@ -207,6 +208,16 @@ public final class IndexScopedSettings extends AbstractScopedSettings {
)
);

/**
* Map of feature flag name to feature-flagged index setting. Once each feature
* is ready for production release, the feature flag can be removed, and the
* setting should be moved to {@link #BUILT_IN_INDEX_SETTINGS}.
*/
public static final Map<String, Setting> FEATURE_FLAGGED_INDEX_SETTINGS = Map.of(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note - Map.of is only available from Java 9+

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be an issue since we have to remain 11+ due to lucene 9.

FeatureFlags.SEGREP_FEATURE_FLAG,
IndexMetadata.INDEX_SEGMENT_REPLICATION_SETTING
);

public static final IndexScopedSettings DEFAULT_SCOPED_SETTINGS = new IndexScopedSettings(Settings.EMPTY, BUILT_IN_INDEX_SETTINGS);

public IndexScopedSettings(Settings settings, Set<Setting<?>> settingsSet) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.opensearch.common.Strings;
import org.opensearch.common.inject.Binder;
import org.opensearch.common.inject.Module;
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.common.xcontent.ToXContent;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentType;
Expand Down Expand Up @@ -85,6 +86,12 @@ public SettingsModule(
registerSetting(setting);
}

for (Map.Entry<String, Setting> featureFlaggedSetting : IndexScopedSettings.FEATURE_FLAGGED_INDEX_SETTINGS.entrySet()) {
if (FeatureFlags.isEnabled(featureFlaggedSetting.getKey())) {
registerSetting(featureFlaggedSetting.getValue());
}
}

for (Setting<?> setting : additionalSettings) {
registerSetting(setting);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/

package org.opensearch.common.util;

/**
* Utility class to manage feature flags. Feature flags are system properties that must be set on the JVM.
* These are used to gate the visibility/availability of incomplete features. Fore more information, see
* https://featureflags.io/feature-flag-introduction/
*/
public class FeatureFlags {

/**
* Gates the visibility of the index setting that enables segment replication.
* Once the feature is ready for production release, this feature flag can be removed.
*/
public static final String SEGREP_FEATURE_FLAG = "opensearch.segment_replication_feature_flag_enabled";
nknize marked this conversation as resolved.
Show resolved Hide resolved

/**
* Used to test feature flags whose values are expected to be booleans.
* This method returns true if the value is "true" (case-insensitive),
* and false otherwise.
*/
public static boolean isEnabled(String featureFlagName) {
return "true".equalsIgnoreCase(System.getProperty(featureFlagName));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return "true".equalsIgnoreCase(System.getProperty(featureFlagName));
return "true".equalsIgnoreCase(System.getProperty(featureFlagName)) || Build.CURRENT.isSnapshot();

It might be nice to auto enable for snapshot builds? Doing this, though, will most certainly fail the FeatureFlag tests the way they are currently written.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,7 @@ public final class IndexSettings {
private final String nodeName;
private final Settings nodeSettings;
private final int numberOfShards;
private final Boolean isSegRepEnabled;
// volatile fields are updated via #updateIndexMetadata(IndexMetadata) under lock
private volatile Settings settings;
private volatile IndexMetadata indexMetadata;
Expand Down Expand Up @@ -681,6 +682,7 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
nodeName = Node.NODE_NAME_SETTING.get(settings);
this.indexMetadata = indexMetadata;
numberOfShards = settings.getAsInt(IndexMetadata.SETTING_NUMBER_OF_SHARDS, null);
isSegRepEnabled = settings.getAsBoolean(IndexMetadata.SETTING_SEGMENT_REPLICATION, false);

this.searchThrottled = INDEX_SEARCH_THROTTLED.get(settings);
this.queryStringLenient = QUERY_STRING_LENIENT_SETTING.get(settings);
Expand Down Expand Up @@ -915,6 +917,13 @@ public int getNumberOfReplicas() {
return settings.getAsInt(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, null);
}

/**
* Returns true if segment replication is enabled on the index.
*/
public boolean isSegrepEnabled() {
return Boolean.TRUE.equals(isSegRepEnabled);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can just be return isSegRepEnabled? Also probably make 'R' uppercase in the method name to be consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I'll stop commenting until you move this out of draft :)

Copy link
Member Author

@kartg kartg Apr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in this method has changed with the latest commit - IndexSettings now stores the replication type itself rather than storing a boolean.

That said, I do like SegRep better than Segrep so i'll change the method name 😏

Also, feel free to leave early comments - I think this change is just about finalized anyway

}

/**
* Returns the node settings. The settings returned from {@link #getSettings()} are a merged version of the
* index settings and the node settings where node settings are overwritten by index settings.
Expand Down