Skip to content

Commit 94dcc1a

Browse files
kh3raAditya Khera
authored and
anandpatel9998
committed
Remove merged segment warmer experimental flag (opensearch-project#19715)
* Removing FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG Signed-off-by: Aditya Khera <kheraadi@amazon.com> # Conflicts: # server/src/main/java/org/opensearch/indices/recovery/RecoverySettings.java # server/src/test/java/org/opensearch/indices/recovery/RecoverySettingsDynamicUpdateTests.java * Added version check to MergedSegmentWarmer Added changelog Fixed tests Signed-off-by: Aditya Khera <kheraadi@amazon.com> * Test fixes Signed-off-by: Aditya Khera <kheraadi@amazon.com> * Bug fix Signed-off-by: Aditya Khera <kheraadi@amazon.com> * Bug fix Signed-off-by: Aditya Khera <kheraadi@amazon.com> --------- Signed-off-by: Aditya Khera <kheraadi@amazon.com> Co-authored-by: Aditya Khera <kheraadi@amazon.com>
1 parent fc1c635 commit 94dcc1a

File tree

19 files changed

+57
-152
lines changed

19 files changed

+57
-152
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
2929
- Wrap checked exceptions in painless.DefBootstrap to support JDK-25 ([#19706](https://github.com/opensearch-project/OpenSearch/pull/19706))
3030
- Refactor the ThreadPoolStats.Stats class to use the Builder pattern instead of constructors ([#19317](https://github.com/opensearch-project/OpenSearch/pull/19317))
3131
- Refactor the IndexingStats.Stats class to use the Builder pattern instead of constructors ([#19306](https://github.com/opensearch-project/OpenSearch/pull/19306))
32-
32+
- Remove FeatureFlag.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG. ([#19715](https://github.com/opensearch-project/OpenSearch/pull/19715))
33+
-
3334
### Fixed
3435
- Fix Allocation and Rebalance Constraints of WeightFunction are incorrectly reset ([#19012](https://github.com/opensearch-project/OpenSearch/pull/19012))
3536
- Fix flaky test FieldDataLoadingIT.testIndicesFieldDataCacheSizeSetting ([#19571](https://github.com/opensearch-project/OpenSearch/pull/19571))

‎server/src/internalClusterTest/java/org/opensearch/indices/replication/MergedSegmentWarmerIT.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import org.opensearch.action.support.WriteRequest;
1919
import org.opensearch.common.settings.Settings;
2020
import org.opensearch.common.unit.TimeValue;
21-
import org.opensearch.common.util.FeatureFlags;
2221
import org.opensearch.common.util.set.Sets;
2322
import org.opensearch.index.IndexSettings;
2423
import org.opensearch.index.TieredMergePolicyProvider;
@@ -55,13 +54,6 @@ protected Settings nodeSettings(int nodeOrdinal) {
5554
.build();
5655
}
5756

58-
@Override
59-
protected Settings featureFlagSettings() {
60-
Settings.Builder featureSettings = Settings.builder();
61-
featureSettings.put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true);
62-
return featureSettings.build();
63-
}
64-
6557
public void testPrimaryNodeRestart() throws Exception {
6658
logger.info("--> start nodes");
6759
internalCluster().startNode();

‎server/src/internalClusterTest/java/org/opensearch/indices/replication/RemoteStoreMergedSegmentWarmerIT.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
import org.opensearch.action.support.WriteRequest;
1414
import org.opensearch.action.support.replication.TransportReplicationAction;
1515
import org.opensearch.common.settings.Settings;
16-
import org.opensearch.common.util.FeatureFlags;
1716
import org.opensearch.index.IndexSettings;
1817
import org.opensearch.index.TieredMergePolicyProvider;
1918
import org.opensearch.indices.recovery.RecoverySettings;
@@ -48,13 +47,6 @@ protected Settings nodeSettings(int nodeOrdinal) {
4847
.build();
4948
}
5049

51-
@Override
52-
protected Settings featureFlagSettings() {
53-
Settings.Builder featureSettings = Settings.builder();
54-
featureSettings.put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true);
55-
return featureSettings.build();
56-
}
57-
5850
@Before
5951
public void setup() {
6052
internalCluster().startClusterManagerOnlyNode();

‎server/src/internalClusterTest/java/org/opensearch/merge/MergeStatsIT.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import org.opensearch.cluster.node.DiscoveryNode;
2525
import org.opensearch.cluster.routing.allocation.decider.ShardsLimitAllocationDecider;
2626
import org.opensearch.common.settings.Settings;
27-
import org.opensearch.common.util.FeatureFlags;
2827
import org.opensearch.core.common.unit.ByteSizeValue;
2928
import org.opensearch.index.merge.MergeStats;
3029
import org.opensearch.index.merge.MergedSegmentWarmerStats;
@@ -65,13 +64,6 @@ public Settings indexSettings() {
6564
.build();
6665
}
6766

68-
@Override
69-
protected Settings featureFlagSettings() {
70-
Settings.Builder featureSettings = Settings.builder();
71-
featureSettings.put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true);
72-
return featureSettings.build();
73-
}
74-
7567
private void setup() {
7668
internalCluster().startNodes(2);
7769
}

‎server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ protected FeatureFlagSettings(
3838
FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES_SETTING,
3939
FeatureFlags.TERM_VERSION_PRECOMMIT_ENABLE_SETTING,
4040
FeatureFlags.ARROW_STREAMS_SETTING,
41-
FeatureFlags.STREAM_TRANSPORT_SETTING,
42-
FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING
41+
FeatureFlags.STREAM_TRANSPORT_SETTING
4342
);
4443
}

‎server/src/main/java/org/opensearch/common/util/FeatureFlags.java‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -63,12 +63,6 @@ public class FeatureFlags {
6363
*/
6464
public static final String BACKGROUND_TASK_EXECUTION_EXPERIMENTAL = FEATURE_FLAG_PREFIX + "task.background.enabled";
6565

66-
/**
67-
* Gates the functionality of merged segment warmer in local/remote segment replication.
68-
* Once the feature is ready for release, this feature flag can be removed.
69-
*/
70-
public static final String MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG = "opensearch.experimental.feature.merged_segment_warmer.enabled";
71-
7266
public static final Setting<Boolean> REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING = Setting.boolSetting(
7367
REMOTE_STORE_MIGRATION_EXPERIMENTAL,
7468
false,
@@ -91,12 +85,6 @@ public class FeatureFlags {
9185
Property.NodeScope
9286
);
9387

94-
public static final Setting<Boolean> MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING = Setting.boolSetting(
95-
MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG,
96-
false,
97-
Property.NodeScope
98-
);
99-
10088
/**
10189
* Gates the functionality of application based configuration templates.
10290
*/
@@ -145,7 +133,6 @@ static class FeatureFlagsImpl {
145133
put(TERM_VERSION_PRECOMMIT_ENABLE_SETTING, TERM_VERSION_PRECOMMIT_ENABLE_SETTING.getDefault(Settings.EMPTY));
146134
put(ARROW_STREAMS_SETTING, ARROW_STREAMS_SETTING.getDefault(Settings.EMPTY));
147135
put(STREAM_TRANSPORT_SETTING, STREAM_TRANSPORT_SETTING.getDefault(Settings.EMPTY));
148-
put(MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING, MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING.getDefault(Settings.EMPTY));
149136
}
150137
};
151138

‎server/src/main/java/org/opensearch/index/IndexService.java‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -343,9 +343,8 @@ public IndexService(
343343
this.retentionLeaseSyncTask = new AsyncRetentionLeaseSyncTask(this);
344344
}
345345
this.asyncReplicationTask = new AsyncReplicationTask(this);
346-
if (FeatureFlags.isEnabled(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING)) {
347-
this.asyncPublishReferencedSegmentsTask = new AsyncPublishReferencedSegmentsTask(this);
348-
}
346+
this.asyncPublishReferencedSegmentsTask = new AsyncPublishReferencedSegmentsTask(this);
347+
349348
this.translogFactorySupplier = translogFactorySupplier;
350349
this.recoverySettings = recoverySettings;
351350
this.remoteStoreSettings = remoteStoreSettings;
@@ -1210,9 +1209,7 @@ public synchronized void updateMetadata(final IndexMetadata currentIndexMetadata
12101209
onRefreshIntervalChange();
12111210
updateFsyncTaskIfNecessary();
12121211
updateReplicationTask();
1213-
if (FeatureFlags.isEnabled(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING)) {
1214-
updatePublishReferencedSegmentsTask();
1215-
}
1212+
updatePublishReferencedSegmentsTask();
12161213
}
12171214

12181215
metadataListeners.forEach(c -> c.accept(newIndexMetadata));

‎server/src/main/java/org/opensearch/index/engine/InternalEngine.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@
8585
import org.opensearch.common.lucene.uid.VersionsAndSeqNoResolver.DocIdAndSeqNo;
8686
import org.opensearch.common.metrics.CounterMetric;
8787
import org.opensearch.common.unit.TimeValue;
88-
import org.opensearch.common.util.FeatureFlags;
8988
import org.opensearch.common.util.concurrent.AbstractRunnable;
9089
import org.opensearch.common.util.concurrent.KeyedLock;
9190
import org.opensearch.common.util.concurrent.ReleasableLock;
@@ -2398,8 +2397,9 @@ private IndexWriterConfig getIndexWriterConfig() {
23982397
if (config().getLeafSorter() != null) {
23992398
iwc.setLeafSorter(config().getLeafSorter()); // The default segment search order
24002399
}
2401-
if (FeatureFlags.isEnabled(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING)
2402-
&& config().getIndexSettings().isSegRepEnabledOrRemoteNode()) {
2400+
IndexSettings indexSettings = config().getIndexSettings();
2401+
if (indexSettings.isDocumentReplication() == false
2402+
&& (indexSettings.isSegRepLocalEnabled() || indexSettings.isRemoteStoreEnabled())) {
24032403
assert null != config().getIndexReaderWarmer();
24042404
iwc.setMergedSegmentWarmer(config().getIndexReaderWarmer());
24052405
}

‎server/src/main/java/org/opensearch/index/engine/MergedSegmentWarmer.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.apache.lucene.index.LeafReader;
1515
import org.apache.lucene.index.SegmentCommitInfo;
1616
import org.apache.lucene.index.SegmentReader;
17+
import org.opensearch.Version;
1718
import org.opensearch.cluster.service.ClusterService;
1819
import org.opensearch.common.logging.Loggers;
1920
import org.opensearch.index.merge.MergedSegmentTransferTracker;
@@ -113,6 +114,12 @@ SegmentCommitInfo segmentCommitInfo(LeafReader leafReader) {
113114

114115
// package-private for tests
115116
boolean shouldWarm(SegmentCommitInfo segmentCommitInfo) throws IOException {
117+
// Min node version check ensures that we only warm, when all nodes expect it
118+
Version minNodeVersion = clusterService.state().nodes().getMinNodeVersion();
119+
if (Version.V_3_4_0.compareTo(minNodeVersion) > 0) {
120+
return false;
121+
}
122+
116123
if (indexShard.getRecoverySettings().isMergedSegmentReplicationWarmerEnabled() == false) {
117124
return false;
118125
}

‎server/src/main/java/org/opensearch/index/engine/MergedSegmentWarmerFactory.java‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
import org.apache.lucene.index.IndexWriter;
1212
import org.opensearch.cluster.service.ClusterService;
1313
import org.opensearch.common.annotation.ExperimentalApi;
14-
import org.opensearch.common.util.FeatureFlags;
1514
import org.opensearch.index.shard.IndexShard;
1615
import org.opensearch.indices.recovery.RecoverySettings;
1716
import org.opensearch.transport.TransportService;
@@ -35,12 +34,10 @@ public MergedSegmentWarmerFactory(TransportService transportService, RecoverySet
3534
}
3635

3736
public IndexWriter.IndexReaderWarmer get(IndexShard shard) {
38-
if (FeatureFlags.isEnabled(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) == false
39-
|| shard.indexSettings().isDocumentReplication()) {
37+
if (shard.indexSettings().isDocumentReplication()) {
4038
// MergedSegmentWarmerFactory#get is called by IndexShard#newEngineConfig on the initialization of a new indexShard and
41-
// in cases of updates to shard state.
42-
// 1. IndexWriter.IndexReaderWarmer should be null when IndexMetadata.INDEX_REPLICATION_TYPE_SETTING == ReplicationType.DOCUMENT
43-
// 2. IndexWriter.IndexReaderWarmer should be null when the FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG == false
39+
// in case of updates to shard state.
40+
// - IndexWriter.IndexReaderWarmer should be null when IndexMetadata.INDEX_REPLICATION_TYPE_SETTING == ReplicationType.DOCUMENT
4441
return null;
4542
} else if (shard.indexSettings().isSegRepLocalEnabled() || shard.indexSettings().isRemoteStoreEnabled()) {
4643
return new MergedSegmentWarmer(transportService, recoverySettings, clusterService, shard);

0 commit comments

Comments
 (0)