Disallowing compression level for lz4 and best_compression codec (#8737)

* Disallowing compression level for lz4 and best_compression codec * setting up codec settings interface for validation Signed-off-by: Sarthak Aggarwal <[email protected]> (cherry picked from commit 82b5f3c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-project · Aug 2, 2023 · f89c785 · f89c785
1 parent d711f66
commit f89c785
Show file tree

Hide file tree

Showing 7 changed files with 332 additions and 10 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Start replication checkpointTimers on primary before segments upload to remote store. ([#8221]()https://github.com/opensearch-project/OpenSearch/pull/8221)
 - Introduce new static cluster setting to control slice computation for concurrent segment search. ([#8847](https://github.com/opensearch-project/OpenSearch/pull/8884))
 - Add configuration for file cache size to max remote data ratio to prevent oversubscription of file cache ([#8606](https://github.com/opensearch-project/OpenSearch/pull/8606))
+- Disallow compression level to be set for default and best_compression index codecs ([#8737]()https://github.com/opensearch-project/OpenSearch/pull/8737)
 
 ### Dependencies
 - Bump `org.apache.logging.log4j:log4j-core` from 2.17.1 to 2.20.0 ([#8307](https://github.com/opensearch-project/OpenSearch/pull/8307))

diff --git a/server/src/internalClusterTest/java/org/opensearch/index/codec/CodecCompressionLevelIT.java b/server/src/internalClusterTest/java/org/opensearch/index/codec/CodecCompressionLevelIT.java
@@ -0,0 +1,178 @@
+/*
+ * 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.index.codec;
+
+import org.apache.logging.log4j.core.util.Throwables;
+import org.opensearch.action.admin.indices.settings.put.UpdateSettingsRequest;
+import org.opensearch.cluster.metadata.IndexMetadata;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.test.OpenSearchIntegTestCase;
+
+import java.util.concurrent.ExecutionException;
+
+import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
+
+@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST)
+public class CodecCompressionLevelIT extends OpenSearchIntegTestCase {
+
+    public void testLuceneCodecsCreateIndexWithCompressionLevel() {
+
+        internalCluster().ensureAtLeastNumDataNodes(1);
+        final String index = "test-index";
+
+        // creating index
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> createIndex(
+                index,
+                Settings.builder()
+                    .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
+                    .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
+                    .put("index.codec", randomFrom(CodecService.DEFAULT_CODEC, CodecService.BEST_COMPRESSION_CODEC))
+                    .put("index.codec.compression_level", randomIntBetween(1, 6))
+                    .build()
+            )
+        );
+
+        createIndex(
+            index,
+            Settings.builder()
+                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
+                .put("index.codec", randomFrom(CodecService.DEFAULT_CODEC, CodecService.BEST_COMPRESSION_CODEC))
+                .build()
+        );
+        ensureGreen(index);
+    }
+
+    public void testZStandardCodecsCreateIndexWithCompressionLevel() {
+
+        internalCluster().ensureAtLeastNumDataNodes(1);
+        final String index = "test-index";
+
+        // creating index
+        createIndex(
+            index,
+            Settings.builder()
+                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
+                .put("index.codec", randomFrom(CodecService.ZSTD_CODEC, CodecService.ZSTD_NO_DICT_CODEC))
+                .put("index.codec.compression_level", randomIntBetween(1, 6))
+                .build()
+        );
+
+        ensureGreen(index);
+    }
+
+    public void testZStandardToLuceneCodecsWithCompressionLevel() throws ExecutionException, InterruptedException {
+
+        internalCluster().ensureAtLeastNumDataNodes(1);
+        final String index = "test-index";
+
+        // creating index
+        createIndex(
+            index,
+            Settings.builder()
+                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
+                .put("index.codec", randomFrom(CodecService.ZSTD_CODEC, CodecService.ZSTD_NO_DICT_CODEC))
+                .put("index.codec.compression_level", randomIntBetween(1, 6))
+                .build()
+        );
+        ensureGreen(index);
+
+        assertAcked(client().admin().indices().prepareClose(index));
+
+        Throwable executionException = expectThrows(
+            ExecutionException.class,
+            () -> client().admin()
+                .indices()
+                .updateSettings(
+                    new UpdateSettingsRequest(index).settings(
+                        Settings.builder().put("index.codec", randomFrom(CodecService.DEFAULT_CODEC, CodecService.BEST_COMPRESSION_CODEC))
+                    )
+                )
+                .get()
+        );
+
+        Throwable rootCause = Throwables.getRootCause(executionException);
+        assertEquals(IllegalArgumentException.class, rootCause.getClass());
+        assertTrue(rootCause.getMessage().startsWith("Compression level cannot be set"));
+
+        assertAcked(
+            client().admin()
+                .indices()
+                .updateSettings(
+                    new UpdateSettingsRequest(index).settings(
+                        Settings.builder()
+                            .put("index.codec", randomFrom(CodecService.DEFAULT_CODEC, CodecService.BEST_COMPRESSION_CODEC))
+                            .put("index.codec.compression_level", (String) null)
+                    )
+                )
+                .get()
+        );
+
+        assertAcked(client().admin().indices().prepareOpen(index));
+        ensureGreen(index);
+    }
+
+    public void testLuceneToZStandardCodecsWithCompressionLevel() throws ExecutionException, InterruptedException {
+
+        internalCluster().ensureAtLeastNumDataNodes(1);
+        final String index = "test-index";
+
+        // creating index
+        createIndex(
+            index,
+            Settings.builder()
+                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
+                .put("index.codec", randomFrom(CodecService.DEFAULT_CODEC, CodecService.BEST_COMPRESSION_CODEC))
+                .build()
+        );
+        ensureGreen(index);
+
+        assertAcked(client().admin().indices().prepareClose(index));
+
+        Throwable executionException = expectThrows(
+            ExecutionException.class,
+            () -> client().admin()
+                .indices()
+                .updateSettings(
+                    new UpdateSettingsRequest(index).settings(
+                        Settings.builder()
+                            .put("index.codec", randomFrom(CodecService.DEFAULT_CODEC, CodecService.BEST_COMPRESSION_CODEC))
+                            .put("index.codec.compression_level", randomIntBetween(1, 6))
+                    )
+                )
+                .get()
+        );
+
+        Throwable rootCause = Throwables.getRootCause(executionException);
+        assertEquals(IllegalArgumentException.class, rootCause.getClass());
+        assertTrue(rootCause.getMessage().startsWith("Compression level cannot be set"));
+
+        assertAcked(
+            client().admin()
+                .indices()
+                .updateSettings(
+                    new UpdateSettingsRequest(index).settings(
+                        Settings.builder()
+                            .put("index.codec", randomFrom(CodecService.ZSTD_CODEC, CodecService.ZSTD_NO_DICT_CODEC))
+                            .put("index.codec.compression_level", randomIntBetween(1, 6))
+                    )
+                )
+                .get()
+        );
+
+        assertAcked(client().admin().indices().prepareOpen(index));
+        ensureGreen(index);
+    }
+
+}
diff --git a/server/src/main/java/org/opensearch/index/codec/CodecSettings.java b/server/src/main/java/org/opensearch/index/codec/CodecSettings.java
@@ -0,0 +1,21 @@
+/*
+ * 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.index.codec;
+
+import org.apache.lucene.codecs.Codec;
+import org.opensearch.common.settings.Setting;
+
+/**
+ * This {@link CodecSettings} allows us to manage the settings with {@link Codec}.
+ *
+ * @opensearch.internal
+ */
+public interface CodecSettings {
+    boolean supports(Setting<?> setting);
+}
diff --git a/server/src/main/java/org/opensearch/index/codec/customcodecs/ZstdCodec.java b/server/src/main/java/org/opensearch/index/codec/customcodecs/ZstdCodec.java
@@ -9,12 +9,15 @@
 package org.opensearch.index.codec.customcodecs;
 
 import org.apache.logging.log4j.Logger;
+import org.opensearch.common.settings.Setting;
+import org.opensearch.index.codec.CodecSettings;
+import org.opensearch.index.engine.EngineConfig;
 import org.opensearch.index.mapper.MapperService;
 
 /**
  * ZstdCodec provides ZSTD compressor using the <a href="https://github.com/luben/zstd-jni">zstd-jni</a> library.
  */
-public class ZstdCodec extends Lucene95CustomCodec {
+public class ZstdCodec extends Lucene95CustomCodec implements CodecSettings {
 
     /**
      * Creates a new ZstdCodec instance with the default compression level.
@@ -41,4 +44,9 @@ public ZstdCodec(MapperService mapperService, Logger logger, int compressionLeve
     public String toString() {
         return getClass().getSimpleName();
     }
+
+    @Override
+    public boolean supports(Setting<?> setting) {
+        return setting.equals(EngineConfig.INDEX_CODEC_COMPRESSION_LEVEL_SETTING);
+    }
 }
diff --git a/server/src/main/java/org/opensearch/index/codec/customcodecs/ZstdNoDictCodec.java b/server/src/main/java/org/opensearch/index/codec/customcodecs/ZstdNoDictCodec.java
@@ -9,12 +9,15 @@
 package org.opensearch.index.codec.customcodecs;
 
 import org.apache.logging.log4j.Logger;
+import org.opensearch.common.settings.Setting;
+import org.opensearch.index.codec.CodecSettings;
+import org.opensearch.index.engine.EngineConfig;
 import org.opensearch.index.mapper.MapperService;
 
 /**
  * ZstdNoDictCodec provides ZSTD compressor without a dictionary support.
  */
-public class ZstdNoDictCodec extends Lucene95CustomCodec {
+public class ZstdNoDictCodec extends Lucene95CustomCodec implements CodecSettings {
 
     /**
      * Creates a new ZstdNoDictCodec instance with the default compression level.
@@ -41,4 +44,9 @@ public ZstdNoDictCodec(MapperService mapperService, Logger logger, int compressi
     public String toString() {
         return getClass().getSimpleName();
     }
+
+    @Override
+    public boolean supports(Setting<?> setting) {
+        return setting.equals(EngineConfig.INDEX_CODEC_COMPRESSION_LEVEL_SETTING);
+    }
 }
diff --git a/server/src/main/java/org/opensearch/index/engine/EngineConfig.java b/server/src/main/java/org/opensearch/index/engine/EngineConfig.java
@@ -48,6 +48,7 @@
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.index.IndexSettings;
 import org.opensearch.index.codec.CodecService;
+import org.opensearch.index.codec.CodecSettings;
 import org.opensearch.index.mapper.ParsedDocument;
 import org.opensearch.index.seqno.RetentionLeases;
 import org.opensearch.core.index.shard.ShardId;
@@ -63,6 +64,7 @@
 import java.util.Comparator;
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
 import java.util.function.BooleanSupplier;
 import java.util.function.LongSupplier;
 import java.util.function.Supplier;
@@ -148,13 +150,52 @@ public Supplier<RetentionLeases> retentionLeasesSupplier() {
      * Compression Level gives a trade-off between compression ratio and speed. The higher compression level results in higher compression ratio but slower compression and decompression speeds.
      * This setting is <b>not</b> realtime updateable.
      */
-    public static final Setting<Integer> INDEX_CODEC_COMPRESSION_LEVEL_SETTING = Setting.intSetting(
+
+    public static final Setting<Integer> INDEX_CODEC_COMPRESSION_LEVEL_SETTING = new Setting<>(
         "index.codec.compression_level",
-        3,
-        1,
-        6,
+        Integer.toString(3),
+        new Setting.IntegerParser(1, 6, "index.codec.compression_level", false),
         Property.IndexScope
-    );
+    ) {
+        @Override
+        public Set<SettingDependency> getSettingsDependencies(String key) {
+            return Set.of(new SettingDependency() {
+                @Override
+                public Setting<String> getSetting() {
+                    return INDEX_CODEC_SETTING;
+                }
+
+                @Override
+                public void validate(String key, Object value, Object dependency) {
+                    if (!(dependency instanceof String)) {
+                        throw new IllegalArgumentException("Codec should be of string type.");
+                    }
+                    doValidateCodecSettings((String) dependency);
+                }
+            });
+        }
+    };
+
+    private static void doValidateCodecSettings(final String codec) {
+        switch (codec) {
+            case "zstd":
+            case "zstd_no_dict":
+                return;
+            case "best_compression":
+            case "lucene_default":
+            case "default":
+                break;
+            default:
+                if (Codec.availableCodecs().contains(codec)) {
+                    Codec luceneCodec = Codec.forName(codec);
+                    if (luceneCodec instanceof CodecSettings
+                        && ((CodecSettings) luceneCodec).supports(INDEX_CODEC_COMPRESSION_LEVEL_SETTING)) {
+                        return;
+                    }
+                }
+        }
+        throw new IllegalArgumentException("Compression level cannot be set for the " + codec + " codec.");
+    }
 
     /**
      * Configures an index to optimize documents with auto generated ids for append only. If this setting is updated from <code>false</code>