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 @@ -70,6 +70,7 @@
import org.opensearch.index.mapper.MapperService;
import org.opensearch.index.seqno.SequenceNumbers;
import org.opensearch.index.shard.ShardId;
import org.opensearch.indices.replication.common.ReplicationType;
import org.opensearch.rest.RestStatus;

import java.io.IOException;
Expand Down Expand Up @@ -260,6 +261,18 @@ public Iterator<Setting<?>> settings() {
Property.IndexScope
);

/**
* Used to specify the replication type for the index. By default, document replication is used.
*/
public static final String SETTING_REPLICATION_TYPE = "index.replication.type";
public static final Setting<ReplicationType> INDEX_REPLICATION_TYPE_SETTING = new Setting<>(
SETTING_REPLICATION_TYPE,
ReplicationType.DOCUMENT.toString(),
ReplicationType::parseString,
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.REPLICATION_TYPE,
IndexMetadata.INDEX_REPLICATION_TYPE_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 allows changing of replication type.
* Once the feature is ready for production release, this feature flag can be removed.
*/
public static final String REPLICATION_TYPE = "opensearch.experimental.feature.replication_type.enabled";

/**
* 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.

}
}
12 changes: 12 additions & 0 deletions server/src/main/java/org/opensearch/index/IndexSettings.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.opensearch.common.unit.ByteSizeValue;
import org.opensearch.common.unit.TimeValue;
import org.opensearch.index.translog.Translog;
import org.opensearch.indices.replication.common.ReplicationType;
import org.opensearch.ingest.IngestService;
import org.opensearch.node.Node;

Expand Down Expand Up @@ -530,6 +531,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 @@ -682,6 +684,9 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
this.indexMetadata = indexMetadata;
numberOfShards = settings.getAsInt(IndexMetadata.SETTING_NUMBER_OF_SHARDS, null);

ReplicationType replicationType = ReplicationType.parseString(settings.get(IndexMetadata.SETTING_REPLICATION_TYPE));
isSegRepEnabled = ReplicationType.SEGMENT.equals(replicationType);

this.searchThrottled = INDEX_SEARCH_THROTTLED.get(settings);
this.queryStringLenient = QUERY_STRING_LENIENT_SETTING.get(settings);
this.queryStringAnalyzeWildcard = QUERY_STRING_ANALYZE_WILDCARD.get(nodeSettings);
Expand Down Expand Up @@ -915,6 +920,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
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* 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.indices.replication.common;

/**
* Enumerates the types of replication strategies supported by OpenSearch.
* For more information, see https://github.com/opensearch-project/OpenSearch/issues/1694
*/
public enum ReplicationType {

DOCUMENT,
SEGMENT;

public static ReplicationType parseString(String replicationType) {
try {
return ReplicationType.valueOf(replicationType);
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException("Could not parse ReplicationStrategy for [" + replicationType + "]");
} catch (NullPointerException npe) {
// return a default value for null input
return DOCUMENT;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* 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;

import org.opensearch.test.OpenSearchTestCase;

public class FeatureFlagTests extends OpenSearchTestCase {
Copy link
Member Author

Choose a reason for hiding this comment

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

I realize that these unit tests are rudimentary and don't actually test the isEnabled == true code path for FeatureFlags. I wasn't able to use System.setProperty - it gets flagged as a forbidden API. I would appreciate suggestions on how I could achieve better test coverage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I pushed a commit that grants write permissions on opensearch.experimental.feature.* so we can test the flags accordingly.


public void testMissingFeatureFlag() {
String testFlag = "missingFeatureFlag";
assertNull(System.getProperty(testFlag));
assertFalse(FeatureFlags.isEnabled(testFlag));
}

public void testNonBooleanFeatureFlag() {
String javaVersionProperty = "java.version";
assertNotNull(System.getProperty(javaVersionProperty));
assertFalse(FeatureFlags.isEnabled(javaVersionProperty));
}
}