diff --git a/CHANGELOG.md b/CHANGELOG.md index d800c93a2e0d5..15fdaae324a8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,7 +28,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Wrap checked exceptions in painless.DefBootstrap to support JDK-25 ([#19706](https://github.com/opensearch-project/OpenSearch/pull/19706)) - Refactor the ThreadPoolStats.Stats class to use the Builder pattern instead of constructors ([#19317](https://github.com/opensearch-project/OpenSearch/pull/19317)) - Refactor the IndexingStats.Stats class to use the Builder pattern instead of constructors ([#19306](https://github.com/opensearch-project/OpenSearch/pull/19306)) - +- Remove FeatureFlag.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG. ([#19715](https://github.com/opensearch-project/OpenSearch/pull/19715)) +- ### Fixed - Fix Allocation and Rebalance Constraints of WeightFunction are incorrectly reset ([#19012](https://github.com/opensearch-project/OpenSearch/pull/19012)) - Fix flaky test FieldDataLoadingIT.testIndicesFieldDataCacheSizeSetting ([#19571](https://github.com/opensearch-project/OpenSearch/pull/19571)) diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/replication/MergedSegmentWarmerIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/replication/MergedSegmentWarmerIT.java index 94cc938b4c128..5420e17bb9807 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/replication/MergedSegmentWarmerIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/replication/MergedSegmentWarmerIT.java @@ -18,7 +18,6 @@ import org.opensearch.action.support.WriteRequest; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.set.Sets; import org.opensearch.index.IndexSettings; import org.opensearch.index.TieredMergePolicyProvider; @@ -55,13 +54,6 @@ protected Settings nodeSettings(int nodeOrdinal) { .build(); } - @Override - protected Settings featureFlagSettings() { - Settings.Builder featureSettings = Settings.builder(); - featureSettings.put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true); - return featureSettings.build(); - } - public void testPrimaryNodeRestart() throws Exception { logger.info("--> start nodes"); internalCluster().startNode(); diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/replication/RemoteStoreMergedSegmentWarmerIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/replication/RemoteStoreMergedSegmentWarmerIT.java index e902b655d0a76..8b9ac33397800 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/replication/RemoteStoreMergedSegmentWarmerIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/replication/RemoteStoreMergedSegmentWarmerIT.java @@ -13,7 +13,6 @@ import org.opensearch.action.support.WriteRequest; import org.opensearch.action.support.replication.TransportReplicationAction; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.IndexSettings; import org.opensearch.index.TieredMergePolicyProvider; import org.opensearch.indices.recovery.RecoverySettings; @@ -48,13 +47,6 @@ protected Settings nodeSettings(int nodeOrdinal) { .build(); } - @Override - protected Settings featureFlagSettings() { - Settings.Builder featureSettings = Settings.builder(); - featureSettings.put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true); - return featureSettings.build(); - } - @Before public void setup() { internalCluster().startClusterManagerOnlyNode(); diff --git a/server/src/internalClusterTest/java/org/opensearch/merge/MergeStatsIT.java b/server/src/internalClusterTest/java/org/opensearch/merge/MergeStatsIT.java index 4aacf7aaf5853..0cd6a1fb4e149 100644 --- a/server/src/internalClusterTest/java/org/opensearch/merge/MergeStatsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/merge/MergeStatsIT.java @@ -24,7 +24,6 @@ import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.routing.allocation.decider.ShardsLimitAllocationDecider; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.index.merge.MergeStats; import org.opensearch.index.merge.MergedSegmentWarmerStats; @@ -65,13 +64,6 @@ public Settings indexSettings() { .build(); } - @Override - protected Settings featureFlagSettings() { - Settings.Builder featureSettings = Settings.builder(); - featureSettings.put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true); - return featureSettings.build(); - } - private void setup() { internalCluster().startNodes(2); } diff --git a/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java index ba6ba1f88b58c..b7c584d449b2f 100644 --- a/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java @@ -38,7 +38,6 @@ protected FeatureFlagSettings( FeatureFlags.APPLICATION_BASED_CONFIGURATION_TEMPLATES_SETTING, FeatureFlags.TERM_VERSION_PRECOMMIT_ENABLE_SETTING, FeatureFlags.ARROW_STREAMS_SETTING, - FeatureFlags.STREAM_TRANSPORT_SETTING, - FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING + FeatureFlags.STREAM_TRANSPORT_SETTING ); } diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index c53922b0e5ceb..8a3086d56a7e1 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -63,12 +63,6 @@ public class FeatureFlags { */ public static final String BACKGROUND_TASK_EXECUTION_EXPERIMENTAL = FEATURE_FLAG_PREFIX + "task.background.enabled"; - /** - * Gates the functionality of merged segment warmer in local/remote segment replication. - * Once the feature is ready for release, this feature flag can be removed. - */ - public static final String MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG = "opensearch.experimental.feature.merged_segment_warmer.enabled"; - public static final Setting REMOTE_STORE_MIGRATION_EXPERIMENTAL_SETTING = Setting.boolSetting( REMOTE_STORE_MIGRATION_EXPERIMENTAL, false, @@ -91,12 +85,6 @@ public class FeatureFlags { Property.NodeScope ); - public static final Setting MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING = Setting.boolSetting( - MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, - false, - Property.NodeScope - ); - /** * Gates the functionality of application based configuration templates. */ @@ -145,7 +133,6 @@ static class FeatureFlagsImpl { put(TERM_VERSION_PRECOMMIT_ENABLE_SETTING, TERM_VERSION_PRECOMMIT_ENABLE_SETTING.getDefault(Settings.EMPTY)); put(ARROW_STREAMS_SETTING, ARROW_STREAMS_SETTING.getDefault(Settings.EMPTY)); put(STREAM_TRANSPORT_SETTING, STREAM_TRANSPORT_SETTING.getDefault(Settings.EMPTY)); - put(MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING, MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING.getDefault(Settings.EMPTY)); } }; diff --git a/server/src/main/java/org/opensearch/index/IndexService.java b/server/src/main/java/org/opensearch/index/IndexService.java index 47a05fbcac820..206298c370676 100644 --- a/server/src/main/java/org/opensearch/index/IndexService.java +++ b/server/src/main/java/org/opensearch/index/IndexService.java @@ -343,9 +343,8 @@ public IndexService( this.retentionLeaseSyncTask = new AsyncRetentionLeaseSyncTask(this); } this.asyncReplicationTask = new AsyncReplicationTask(this); - if (FeatureFlags.isEnabled(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING)) { - this.asyncPublishReferencedSegmentsTask = new AsyncPublishReferencedSegmentsTask(this); - } + this.asyncPublishReferencedSegmentsTask = new AsyncPublishReferencedSegmentsTask(this); + this.translogFactorySupplier = translogFactorySupplier; this.recoverySettings = recoverySettings; this.remoteStoreSettings = remoteStoreSettings; @@ -1210,9 +1209,7 @@ public synchronized void updateMetadata(final IndexMetadata currentIndexMetadata onRefreshIntervalChange(); updateFsyncTaskIfNecessary(); updateReplicationTask(); - if (FeatureFlags.isEnabled(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING)) { - updatePublishReferencedSegmentsTask(); - } + updatePublishReferencedSegmentsTask(); } metadataListeners.forEach(c -> c.accept(newIndexMetadata)); diff --git a/server/src/main/java/org/opensearch/index/engine/InternalEngine.java b/server/src/main/java/org/opensearch/index/engine/InternalEngine.java index e154c69fabf81..08a4e9c3f788c 100644 --- a/server/src/main/java/org/opensearch/index/engine/InternalEngine.java +++ b/server/src/main/java/org/opensearch/index/engine/InternalEngine.java @@ -85,7 +85,6 @@ import org.opensearch.common.lucene.uid.VersionsAndSeqNoResolver.DocIdAndSeqNo; import org.opensearch.common.metrics.CounterMetric; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.AbstractRunnable; import org.opensearch.common.util.concurrent.KeyedLock; import org.opensearch.common.util.concurrent.ReleasableLock; @@ -2398,8 +2397,9 @@ private IndexWriterConfig getIndexWriterConfig() { if (config().getLeafSorter() != null) { iwc.setLeafSorter(config().getLeafSorter()); // The default segment search order } - if (FeatureFlags.isEnabled(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_SETTING) - && config().getIndexSettings().isSegRepEnabledOrRemoteNode()) { + IndexSettings indexSettings = config().getIndexSettings(); + if (indexSettings.isDocumentReplication() == false + && (indexSettings.isSegRepLocalEnabled() || indexSettings.isRemoteStoreEnabled())) { assert null != config().getIndexReaderWarmer(); iwc.setMergedSegmentWarmer(config().getIndexReaderWarmer()); } diff --git a/server/src/main/java/org/opensearch/index/engine/MergedSegmentWarmer.java b/server/src/main/java/org/opensearch/index/engine/MergedSegmentWarmer.java index 7be57d5b7bfa6..c85298d970d4b 100644 --- a/server/src/main/java/org/opensearch/index/engine/MergedSegmentWarmer.java +++ b/server/src/main/java/org/opensearch/index/engine/MergedSegmentWarmer.java @@ -14,6 +14,7 @@ import org.apache.lucene.index.LeafReader; import org.apache.lucene.index.SegmentCommitInfo; import org.apache.lucene.index.SegmentReader; +import org.opensearch.Version; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.logging.Loggers; import org.opensearch.index.merge.MergedSegmentTransferTracker; @@ -113,6 +114,12 @@ SegmentCommitInfo segmentCommitInfo(LeafReader leafReader) { // package-private for tests boolean shouldWarm(SegmentCommitInfo segmentCommitInfo) throws IOException { + // Min node version check ensures that we only warm, when all nodes expect it + Version minNodeVersion = clusterService.state().nodes().getMinNodeVersion(); + if (Version.V_3_4_0.compareTo(minNodeVersion) > 0) { + return false; + } + if (indexShard.getRecoverySettings().isMergedSegmentReplicationWarmerEnabled() == false) { return false; } diff --git a/server/src/main/java/org/opensearch/index/engine/MergedSegmentWarmerFactory.java b/server/src/main/java/org/opensearch/index/engine/MergedSegmentWarmerFactory.java index 5eda7a53c5660..98f7767e8a0ae 100644 --- a/server/src/main/java/org/opensearch/index/engine/MergedSegmentWarmerFactory.java +++ b/server/src/main/java/org/opensearch/index/engine/MergedSegmentWarmerFactory.java @@ -11,7 +11,6 @@ import org.apache.lucene.index.IndexWriter; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.annotation.ExperimentalApi; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.shard.IndexShard; import org.opensearch.indices.recovery.RecoverySettings; import org.opensearch.transport.TransportService; @@ -35,12 +34,10 @@ public MergedSegmentWarmerFactory(TransportService transportService, RecoverySet } public IndexWriter.IndexReaderWarmer get(IndexShard shard) { - if (FeatureFlags.isEnabled(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) == false - || shard.indexSettings().isDocumentReplication()) { + if (shard.indexSettings().isDocumentReplication()) { // MergedSegmentWarmerFactory#get is called by IndexShard#newEngineConfig on the initialization of a new indexShard and - // in cases of updates to shard state. - // 1. IndexWriter.IndexReaderWarmer should be null when IndexMetadata.INDEX_REPLICATION_TYPE_SETTING == ReplicationType.DOCUMENT - // 2. IndexWriter.IndexReaderWarmer should be null when the FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG == false + // in case of updates to shard state. + // - IndexWriter.IndexReaderWarmer should be null when IndexMetadata.INDEX_REPLICATION_TYPE_SETTING == ReplicationType.DOCUMENT return null; } else if (shard.indexSettings().isSegRepLocalEnabled() || shard.indexSettings().isRemoteStoreEnabled()) { return new MergedSegmentWarmer(transportService, recoverySettings, clusterService, shard); diff --git a/server/src/main/java/org/opensearch/indices/recovery/RecoverySettings.java b/server/src/main/java/org/opensearch/indices/recovery/RecoverySettings.java index 3975742e15e52..bb7e5dd5cfd28 100644 --- a/server/src/main/java/org/opensearch/indices/recovery/RecoverySettings.java +++ b/server/src/main/java/org/opensearch/indices/recovery/RecoverySettings.java @@ -42,7 +42,6 @@ import org.opensearch.common.settings.Setting.Property; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.OpenSearchExecutors; import org.opensearch.core.common.unit.ByteSizeUnit; import org.opensearch.core.common.unit.ByteSizeValue; @@ -93,13 +92,6 @@ public class RecoverySettings { public static final Setting INDICES_MERGED_SEGMENT_REPLICATION_WARMER_ENABLED_SETTING = Setting.boolSetting( "indices.replication.merges.warmer.enabled", false, - value -> { - if (FeatureFlags.isEnabled(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) == false && value == true) { - throw new IllegalArgumentException( - "FeatureFlag " + FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG + " must be enabled to set this property to true." - ); - } - }, Property.Dynamic, Property.NodeScope ); diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index f81db57250e56..fe74e9e91c1ec 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -1682,16 +1682,10 @@ protected Node(final Environment initialEnvironment, Collection clas b.bind(MergedSegmentWarmerFactory.class).toInstance(mergedSegmentWarmerFactory); b.bind(MappingTransformerRegistry.class).toInstance(mappingTransformerRegistry); b.bind(AutoForceMergeManager.class).toInstance(autoForceMergeManager); - if (FeatureFlags.isEnabled(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG)) { - if (isRemoteDataAttributePresent(settings)) { - b.bind(MergedSegmentPublisher.PublishAction.class) - .to(RemoteStorePublishMergedSegmentAction.class) - .asEagerSingleton(); - } else { - b.bind(MergedSegmentPublisher.PublishAction.class).to(PublishMergedSegmentAction.class).asEagerSingleton(); - } + if (isRemoteDataAttributePresent(settings)) { + b.bind(MergedSegmentPublisher.PublishAction.class).to(RemoteStorePublishMergedSegmentAction.class).asEagerSingleton(); } else { - b.bind(MergedSegmentPublisher.PublishAction.class).toInstance((shard, checkpoint) -> {}); + b.bind(MergedSegmentPublisher.PublishAction.class).to(PublishMergedSegmentAction.class).asEagerSingleton(); } b.bind(MergedSegmentPublisher.class).asEagerSingleton(); diff --git a/server/src/test/java/org/opensearch/index/IndexServiceTests.java b/server/src/test/java/org/opensearch/index/IndexServiceTests.java index 071b4e6a9d55f..793b745cbe434 100644 --- a/server/src/test/java/org/opensearch/index/IndexServiceTests.java +++ b/server/src/test/java/org/opensearch/index/IndexServiceTests.java @@ -73,7 +73,6 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; -import static org.opensearch.common.util.FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG; import static org.opensearch.index.shard.IndexShardTestCase.getEngine; import static org.opensearch.test.InternalSettingsPlugin.TRANSLOG_RETENTION_CHECK_INTERVAL_SETTING; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; @@ -619,7 +618,6 @@ public void testReplicationTask() throws Exception { assertEquals(1000, updatedTask.getInterval().millis()); } - @LockFeatureFlag(MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) public void testPublishReferencedSegmentsTask() throws Exception { // create with docrep - task should not schedule IndexService indexService = createIndex( diff --git a/server/src/test/java/org/opensearch/index/engine/MergedSegmentWarmerFactoryTests.java b/server/src/test/java/org/opensearch/index/engine/MergedSegmentWarmerFactoryTests.java index ce84681b2b64e..b3883e2f916cf 100644 --- a/server/src/test/java/org/opensearch/index/engine/MergedSegmentWarmerFactoryTests.java +++ b/server/src/test/java/org/opensearch/index/engine/MergedSegmentWarmerFactoryTests.java @@ -54,8 +54,7 @@ public void tearDown() throws Exception { super.tearDown(); } - public void testGetWithSegmentReplicationAndExperimentalFeatureFlagEnabled() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true).build()); + public void testGetWithSegmentReplication() { IndexSettings indexSettings = createIndexSettings( false, Settings.builder().put(IndexMetadata.INDEX_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT).build() @@ -67,19 +66,7 @@ public void testGetWithSegmentReplicationAndExperimentalFeatureFlagEnabled() { assertTrue(warmer instanceof MergedSegmentWarmer); } - public void testGetWithSegmentReplicationAndExperimentalFeatureFlagDisabled() { - IndexSettings indexSettings = createIndexSettings( - false, - Settings.builder().put(IndexMetadata.INDEX_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT).build() - ); - when(indexShard.indexSettings()).thenReturn(indexSettings); - IndexWriter.IndexReaderWarmer warmer = factory.get(indexShard); - - assertNull(warmer); - } - - public void testGetWithRemoteStoreEnabledAndExperimentalFeatureFlagEnabled() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true).build()); + public void testGetWithRemoteStoreEnabled() { IndexSettings indexSettings = createIndexSettings( true, Settings.builder().put(IndexMetadata.INDEX_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT).build() @@ -92,32 +79,7 @@ public void testGetWithRemoteStoreEnabledAndExperimentalFeatureFlagEnabled() { assertTrue(warmer instanceof MergedSegmentWarmer); } - public void testGetWithRemoteStoreEnabledAndExperimentalFeatureFlagDisabled() { - IndexSettings indexSettings = createIndexSettings( - true, - Settings.builder().put(IndexMetadata.INDEX_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT).build() - ); - when(indexShard.indexSettings()).thenReturn(indexSettings); - - IndexWriter.IndexReaderWarmer warmer = factory.get(indexShard); - - assertNull(warmer); - } - - public void testGetWithDocumentReplicationAndExperimentalFeatureFlagEnabled() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true).build()); - IndexSettings indexSettings = createIndexSettings( - false, - Settings.builder().put(IndexMetadata.INDEX_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.DOCUMENT).build() - ); - - when(indexShard.indexSettings()).thenReturn(indexSettings); - IndexWriter.IndexReaderWarmer warmer = factory.get(indexShard); - - assertNull(warmer); - } - - public void testGetWithDocumentReplicationAndExperimentalFeatureFlagDisabled() { + public void testGetWithDocumentReplication() { IndexSettings indexSettings = createIndexSettings( false, Settings.builder().put(IndexMetadata.INDEX_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.DOCUMENT).build() @@ -125,7 +87,6 @@ public void testGetWithDocumentReplicationAndExperimentalFeatureFlagDisabled() { when(indexShard.indexSettings()).thenReturn(indexSettings); IndexWriter.IndexReaderWarmer warmer = factory.get(indexShard); - assertNull(warmer); } diff --git a/server/src/test/java/org/opensearch/index/engine/MergedSegmentWarmerTests.java b/server/src/test/java/org/opensearch/index/engine/MergedSegmentWarmerTests.java index 8318f62e40d6f..65f6c398b2d6f 100644 --- a/server/src/test/java/org/opensearch/index/engine/MergedSegmentWarmerTests.java +++ b/server/src/test/java/org/opensearch/index/engine/MergedSegmentWarmerTests.java @@ -17,6 +17,9 @@ import org.apache.lucene.tests.store.MockDirectoryWrapper; import org.apache.lucene.util.StringHelper; import org.opensearch.Version; +import org.opensearch.cluster.ClusterState; +import org.opensearch.cluster.node.DiscoveryNodes; +import org.opensearch.cluster.service.ClusterService; import org.opensearch.core.common.unit.ByteSizeUnit; import org.opensearch.core.common.unit.ByteSizeValue; import org.opensearch.core.index.Index; @@ -50,15 +53,25 @@ public class MergedSegmentWarmerTests extends OpenSearchTestCase { SegmentCommitInfo segmentCommitInfo; @Mock MergedSegmentTransferTracker mergedSegmentTransferTracker; + @Mock + DiscoveryNodes mockDiscoveryNodes; @Override public void setUp() throws Exception { super.setUp(); mockIndexShard = mock(IndexShard.class); mergedSegmentTransferTracker = mock(MergedSegmentTransferTracker.class); + + ClusterState mockClusterState = mock(ClusterState.class); + mockDiscoveryNodes = mock(DiscoveryNodes.class); + ClusterService mockClusterService = mock(ClusterService.class); + when(mockClusterService.state()).thenReturn(mockClusterState); + when(mockClusterState.nodes()).thenReturn(mockDiscoveryNodes); + when(mockDiscoveryNodes.getMinNodeVersion()).thenReturn(Version.V_3_4_0); + when(mockIndexShard.mergedSegmentTransferTracker()).thenReturn(mergedSegmentTransferTracker); when(mockIndexShard.shardId()).thenReturn(new ShardId(new Index("test-index", "_na_"), 0)); - mergedSegmentWarmer = new MergedSegmentWarmer(null, null, null, mockIndexShard); + mergedSegmentWarmer = new MergedSegmentWarmer(null, null, mockClusterService, mockIndexShard); segmentCommitInfo = new SegmentCommitInfo(segmentInfo(), 0, 0, 0, 0, 0, null); } @@ -102,14 +115,28 @@ public void testShouldWarm_segmentSizeLessThanThreshold() throws IOException { } public void testShouldWarm_segmentSizeGreaterThanThreshold() throws IOException { + RecoverySettings mockRecoverySettings = mock(RecoverySettings.class); + when(mockRecoverySettings.isMergedSegmentReplicationWarmerEnabled()).thenReturn(true); + when(mockRecoverySettings.getMergedSegmentWarmerMinSegmentSizeThreshold()).thenReturn(new ByteSizeValue(500, ByteSizeUnit.MB)); + when(mockIndexShard.getRecoverySettings()).thenReturn(mockRecoverySettings); + when(segmentCommitInfo.info.dir.fileLength(any())).thenReturn(150 * 1_000_000L); + + assertFalse( + "MergedSegmentWarmer#shouldWarm is expected to return true when merged segment warmer is enabled", + mergedSegmentWarmer.shouldWarm(segmentCommitInfo) + ); + } + + public void testShouldWarm_minNodeVersionNotSatisfied() throws IOException { RecoverySettings mockRecoverySettings = mock(RecoverySettings.class); when(mockRecoverySettings.isMergedSegmentReplicationWarmerEnabled()).thenReturn(true); when(mockRecoverySettings.getMergedSegmentWarmerMinSegmentSizeThreshold()).thenReturn(new ByteSizeValue(500, ByteSizeUnit.MB)); when(mockIndexShard.getRecoverySettings()).thenReturn(mockRecoverySettings); when(segmentCommitInfo.info.dir.fileLength(any())).thenReturn(600 * 1_000_000L); + when(mockDiscoveryNodes.getMinNodeVersion()).thenReturn(Version.V_3_3_0); - assertTrue( - "MergedSegmentWarmer#shouldWarm is expected to return false when merged segment warmer is disabled", + assertFalse( + "MergedSegmentWarmer#shouldWarm is expected to return false when min node version requirements are not satisfied", mergedSegmentWarmer.shouldWarm(segmentCommitInfo) ); } diff --git a/server/src/test/java/org/opensearch/index/shard/RemoteIndexShardTests.java b/server/src/test/java/org/opensearch/index/shard/RemoteIndexShardTests.java index 5361d878b618e..d2a07546fe68a 100644 --- a/server/src/test/java/org/opensearch/index/shard/RemoteIndexShardTests.java +++ b/server/src/test/java/org/opensearch/index/shard/RemoteIndexShardTests.java @@ -60,7 +60,6 @@ import java.util.function.BiConsumer; import java.util.stream.Collectors; -import static org.opensearch.common.util.FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG; import static org.opensearch.index.engine.EngineTestCase.assertAtMostOneLuceneDocumentPerSequenceNumber; import static org.opensearch.index.shard.RemoteStoreRefreshListener.EXCLUDE_FILES; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -629,7 +628,6 @@ public void testShallowCopySnapshotForClosedIndexSuccessful() throws Exception { } } - @LockFeatureFlag(MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) @Override @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/pull/18255") public void testMergedSegmentReplication() throws Exception { @@ -637,7 +635,6 @@ public void testMergedSegmentReplication() throws Exception { super.testMergedSegmentReplication(); } - @LockFeatureFlag(MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) @Override @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/pull/19436") public void testMergedSegmentReplicationWithException() throws Exception { @@ -645,7 +642,6 @@ public void testMergedSegmentReplicationWithException() throws Exception { super.testMergedSegmentReplicationWithException(); } - @LockFeatureFlag(MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) @Override @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/pull/18255") public void testMergedSegmentReplicationWithZeroReplica() throws Exception { @@ -653,7 +649,6 @@ public void testMergedSegmentReplicationWithZeroReplica() throws Exception { super.testMergedSegmentReplicationWithZeroReplica(); } - @LockFeatureFlag(MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) @Override @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/pull/18720") public void testCleanupRedundantPendingMergeSegment() throws Exception { diff --git a/server/src/test/java/org/opensearch/index/shard/SegmentReplicationIndexShardTests.java b/server/src/test/java/org/opensearch/index/shard/SegmentReplicationIndexShardTests.java index 5898d54e90124..8666e222c9dec 100644 --- a/server/src/test/java/org/opensearch/index/shard/SegmentReplicationIndexShardTests.java +++ b/server/src/test/java/org/opensearch/index/shard/SegmentReplicationIndexShardTests.java @@ -98,7 +98,6 @@ import java.util.function.BiConsumer; import java.util.function.Function; -import static org.opensearch.common.util.FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG; import static org.opensearch.index.engine.EngineTestCase.assertAtMostOneLuceneDocumentPerSequenceNumber; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasToString; @@ -157,7 +156,6 @@ public void testReplication() throws Exception { } } - @LockFeatureFlag(MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) @TestLogging(reason = "Getting trace logs from MergedSegmentWarmer", value = "org.opensearch.index.engine.MergedSegmentWarmer:TRACE") public void testMergedSegmentReplication() throws Exception { // Test that the pre-copy merged segment logic does not block the merge process of the primary shard when there are 1 replica shard. @@ -201,7 +199,6 @@ public void testMergedSegmentReplication() throws Exception { } } - @LockFeatureFlag(MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) public void testMergedSegmentReplicationWithException() throws Exception { // Test that the pre-copy merged segment exception will not cause primary shard to fail MergedSegmentPublisher mergedSegmentPublisherWithException = new MergedSegmentPublisher((indexShard, checkpoint) -> { @@ -247,7 +244,6 @@ public void testMergedSegmentReplicationWithException() throws Exception { } } - @LockFeatureFlag(MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) public void testMergedSegmentReplicationWithZeroReplica() throws Exception { // Test that the pre-copy merged segment logic does not block the merge process of the primary shard when there are 0 replica shard. final RecoverySettings recoverySettings = new RecoverySettings( @@ -283,7 +279,6 @@ public void testMergedSegmentReplicationWithZeroReplica() throws Exception { } } - @LockFeatureFlag(MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG) public void testCleanupRedundantPendingMergeSegment() throws Exception { final RecoverySettings recoverySettings = new RecoverySettings( Settings.builder().put(RecoverySettings.INDICES_MERGED_SEGMENT_REPLICATION_WARMER_ENABLED_SETTING.getKey(), true).build(), diff --git a/server/src/test/java/org/opensearch/indices/recovery/RecoverySettingsDynamicUpdateTests.java b/server/src/test/java/org/opensearch/indices/recovery/RecoverySettingsDynamicUpdateTests.java index b7753a8d35bdf..7a0932e4eeaae 100644 --- a/server/src/test/java/org/opensearch/indices/recovery/RecoverySettingsDynamicUpdateTests.java +++ b/server/src/test/java/org/opensearch/indices/recovery/RecoverySettingsDynamicUpdateTests.java @@ -188,8 +188,6 @@ public void testInternalActionRetryTimeout() { } public void testMergedSegmentReplicationWarmerEnabledSetting() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true).build()); - clusterSettings.applySettings( Settings.builder().put(RecoverySettings.INDICES_MERGED_SEGMENT_REPLICATION_WARMER_ENABLED_SETTING.getKey(), true).build() ); @@ -201,28 +199,7 @@ public void testMergedSegmentReplicationWarmerEnabledSetting() { assertFalse(recoverySettings.isMergedSegmentReplicationWarmerEnabled()); } - public void testMergedSegmentReplicationWarmerEnabledSettingInvalidUpdate() { - Exception e = assertThrows( - IllegalArgumentException.class, - () -> clusterSettings.applySettings( - Settings.builder().put(RecoverySettings.INDICES_MERGED_SEGMENT_REPLICATION_WARMER_ENABLED_SETTING.getKey(), true).build() - ) - ); - assertEquals( - "illegal value can't update [" - + RecoverySettings.INDICES_MERGED_SEGMENT_REPLICATION_WARMER_ENABLED_SETTING.getKey() - + "] from [false] to [true]", - e.getMessage() - ); - assertEquals(IllegalArgumentException.class, e.getCause().getClass()); - assertEquals( - "FeatureFlag opensearch.experimental.feature.merged_segment_warmer.enabled must be enabled to set this property to true.", - e.getCause().getMessage() - ); - } - public void testMergedSegmentWarmerSegmentSizeThresholdSetting() { - FeatureFlags.initializeFeatureFlags(Settings.builder().put(FeatureFlags.MERGED_SEGMENT_WARMER_EXPERIMENTAL_FLAG, true).build()); assertEquals(500L, recoverySettings.getMergedSegmentWarmerMinSegmentSizeThreshold().getMb()); diff --git a/test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java b/test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java index fe6e38e1b3e48..b700a6bf4bb67 100644 --- a/test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java +++ b/test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java @@ -161,6 +161,7 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.notNullValue; +import static org.mockito.Mockito.mock; public abstract class EngineTestCase extends OpenSearchTestCase { @@ -1030,6 +1031,7 @@ protected EngineConfig config( .tombstoneDocSupplier(config.getTombstoneDocSupplier()) .documentMapperForTypeSupplier(documentMapperForTypeSupplier) .clusterApplierService(clusterApplierService) + .indexReaderWarmer(mock(MergedSegmentWarmer.class)) .build(); }