From 45665dbf0c0dd47aa09eef22eb3bdd934aeddd43 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Thu, 17 Feb 2022 11:32:05 -0500 Subject: [PATCH] MapperService has to be passed in as null for EnginePlugins CodecService constructor Signed-off-by: Andriy Redko --- .../index/codec/CodecServiceConfig.java | 37 ++++++++++ .../index/codec/CodecServiceFactory.java | 22 ++++++ .../index/engine/EngineConfigFactory.java | 50 ++++++++++++-- .../opensearch/indices/IndicesService.java | 3 +- .../org/opensearch/plugins/EnginePlugin.java | 16 +++++ .../engine/EngineConfigFactoryTests.java | 68 ++++++++++++++++++- 6 files changed, 186 insertions(+), 10 deletions(-) create mode 100644 server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java create mode 100644 server/src/main/java/org/opensearch/index/codec/CodecServiceFactory.java diff --git a/server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java b/server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java new file mode 100644 index 0000000000000..25e3079ec6be4 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java @@ -0,0 +1,37 @@ +/* + * 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.opensearch.common.Nullable; +import org.opensearch.index.IndexSettings; +import org.opensearch.index.mapper.MapperService; + +import java.util.Objects; + +/** + * The configuration parameters necessary for the {@link CodecService} instance construction. + */ +public final class CodecServiceConfig { + private final IndexSettings indexSettings; + private final MapperService mapperService; + + public CodecServiceConfig(IndexSettings indexSettings, @Nullable MapperService mapperService) { + this.indexSettings = Objects.requireNonNull(indexSettings); + this.mapperService = mapperService; + } + + public IndexSettings getIndexSettings() { + return indexSettings; + } + + @Nullable + public MapperService getMapperService() { + return mapperService; + } +} diff --git a/server/src/main/java/org/opensearch/index/codec/CodecServiceFactory.java b/server/src/main/java/org/opensearch/index/codec/CodecServiceFactory.java new file mode 100644 index 0000000000000..da28c5f06b035 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/codec/CodecServiceFactory.java @@ -0,0 +1,22 @@ +/* + * 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; + +/** + * A factory for creating new {@link CodecService} instance + */ +@FunctionalInterface +public interface CodecServiceFactory { + /** + * Create new {@link CodecService} instance + * @param config code service configuration + * @return new {@link CodecService} instance + */ + CodecService createCodecService(CodecServiceConfig config); +} diff --git a/server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java b/server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java index dffdb58bfec1e..2c4d5d132028b 100644 --- a/server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java +++ b/server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java @@ -18,6 +18,9 @@ import org.opensearch.common.unit.TimeValue; import org.opensearch.index.IndexSettings; import org.opensearch.index.codec.CodecService; +import org.opensearch.index.codec.CodecServiceConfig; +import org.opensearch.index.codec.CodecServiceFactory; +import org.opensearch.index.mapper.MapperService; import org.opensearch.index.seqno.RetentionLeases; import org.opensearch.index.shard.ShardId; import org.opensearch.index.store.Store; @@ -44,20 +47,29 @@ public class EngineConfigFactory { /** default ctor primarily used for tests without plugins */ public EngineConfigFactory(IndexSettings idxSettings) { - this(Collections.emptyList(), idxSettings); + this(Collections.emptyList(), idxSettings, null); } /** * Construct a factory using the plugin service and provided index settings */ public EngineConfigFactory(PluginsService pluginsService, IndexSettings idxSettings) { - this(pluginsService.filterPlugins(EnginePlugin.class), idxSettings); + this(pluginsService.filterPlugins(EnginePlugin.class), idxSettings, null); + } + + /** + * Construct a factory using the plugin service, provided index settings and mapper service + */ + public EngineConfigFactory(PluginsService pluginsService, IndexSettings idxSettings, MapperService mapperService) { + this(pluginsService.filterPlugins(EnginePlugin.class), idxSettings, mapperService); } /* private constructor to construct the factory from specific EnginePlugins and IndexSettings */ - EngineConfigFactory(Collection enginePlugins, IndexSettings idxSettings) { + EngineConfigFactory(Collection enginePlugins, IndexSettings idxSettings, MapperService mapperService) { Optional codecService = Optional.empty(); String codecServiceOverridingPlugin = null; + Optional codecServiceFactory = Optional.empty(); + String codecServiceFactoryOverridingPlugin = null; Optional translogDeletionPolicyFactory = Optional.empty(); String translogDeletionPolicyOverridingPlugin = null; for (EnginePlugin enginePlugin : enginePlugins) { @@ -65,7 +77,7 @@ public EngineConfigFactory(PluginsService pluginsService, IndexSettings idxSetti if (codecService.isPresent() == false) { codecService = enginePlugin.getCustomCodecService(idxSettings); codecServiceOverridingPlugin = enginePlugin.getClass().getName(); - } else { + } else if (enginePlugin.getCustomCodecService(idxSettings).isPresent()) { throw new IllegalStateException( "existing codec service already overridden in: " + codecServiceOverridingPlugin @@ -76,7 +88,7 @@ public EngineConfigFactory(PluginsService pluginsService, IndexSettings idxSetti if (translogDeletionPolicyFactory.isPresent() == false) { translogDeletionPolicyFactory = enginePlugin.getCustomTranslogDeletionPolicyFactory(); translogDeletionPolicyOverridingPlugin = enginePlugin.getClass().getName(); - } else { + } else if (enginePlugin.getCustomTranslogDeletionPolicyFactory().isPresent()) { throw new IllegalStateException( "existing TranslogDeletionPolicyFactory is already overridden in: " + translogDeletionPolicyOverridingPlugin @@ -84,8 +96,34 @@ public EngineConfigFactory(PluginsService pluginsService, IndexSettings idxSetti + enginePlugin.getClass().getName() ); } + // get overriding CodecServiceFactory from EnginePlugin + if (codecServiceFactory.isPresent() == false) { + codecServiceFactory = enginePlugin.getCustomCodecServiceFactory(); + codecServiceFactoryOverridingPlugin = enginePlugin.getClass().getName(); + } else if (enginePlugin.getCustomCodecServiceFactory().isPresent()) { + throw new IllegalStateException( + "existing codec service factory already overridden in: " + + codecServiceFactoryOverridingPlugin + + " attempting to override again by: " + + enginePlugin.getClass().getName() + ); + } + } + + if (codecService.isPresent() && codecServiceFactory.isPresent()) { + throw new IllegalStateException( + "both codec service and codec service factory are present, codec service provided by: " + + codecServiceOverridingPlugin + + " conflicts with codec service factory provided by: " + + codecServiceFactoryOverridingPlugin + ); } - this.codecService = codecService.orElse(null); + + this.codecService = codecService.isPresent() ? codecService.get() : codecServiceFactory.map(factory -> { + final CodecServiceConfig config = new CodecServiceConfig(idxSettings, mapperService); + return factory.createCodecService(config); + }).orElse(null); + this.translogDeletionPolicyFactory = translogDeletionPolicyFactory.orElse((idxs, rtls) -> null); } diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 392bda21a0309..2b6fcf4d62b88 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -742,7 +742,8 @@ private synchronized IndexService createIndexService( } private EngineConfigFactory getEngineConfigFactory(final IndexSettings idxSettings) { - return new EngineConfigFactory(this.pluginsService, idxSettings); + final IndexService indexService = indices.get(idxSettings.getUUID()); + return new EngineConfigFactory(this.pluginsService, idxSettings, (indexService != null) ? indexService.mapperService() : null); } private EngineFactory getEngineFactory(final IndexSettings idxSettings) { diff --git a/server/src/main/java/org/opensearch/plugins/EnginePlugin.java b/server/src/main/java/org/opensearch/plugins/EnginePlugin.java index ee285e8be8c2f..c107ef35044f5 100644 --- a/server/src/main/java/org/opensearch/plugins/EnginePlugin.java +++ b/server/src/main/java/org/opensearch/plugins/EnginePlugin.java @@ -34,6 +34,7 @@ import org.opensearch.index.IndexSettings; import org.opensearch.index.codec.CodecService; +import org.opensearch.index.codec.CodecServiceFactory; import org.opensearch.index.engine.EngineFactory; import org.opensearch.index.seqno.RetentionLeases; import org.opensearch.index.translog.TranslogDeletionPolicy; @@ -63,11 +64,26 @@ public interface EnginePlugin { * to determine if a custom {@link CodecService} should be provided for the given index. A plugin that is not overriding * the {@link CodecService} through the plugin can ignore this method and the Codec specified in the {@link IndexSettings} * will be used. + * + * @deprecated Please use {@code getCustomCodecServiceFactory()} instead as it provides more context for {@link CodecService} + * instance construction. */ + @Deprecated default Optional getCustomCodecService(IndexSettings indexSettings) { return Optional.empty(); } + /** + * EXPERT: + * When an index is created this method is invoked for each engine plugin. Engine plugins can inspect the index settings + * to determine if a custom {@link CodecServiceFactory} should be provided for the given index. A plugin that is not overriding + * the {@link CodecServiceFactory} through the plugin can ignore this method and the default Codec specified in the + * {@link IndexSettings} will be used. + */ + default Optional getCustomCodecServiceFactory() { + return Optional.empty(); + } + /** * When an index is created this method is invoked for each engine plugin. Engine plugins that need to provide a * custom {@link TranslogDeletionPolicy} can override this method to return a function that takes the {@link IndexSettings} diff --git a/server/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.java b/server/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.java index df3e8deb6d90a..73779f11a1650 100644 --- a/server/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.java +++ b/server/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.java @@ -14,6 +14,7 @@ import org.opensearch.common.unit.TimeValue; import org.opensearch.index.IndexSettings; import org.opensearch.index.codec.CodecService; +import org.opensearch.index.codec.CodecServiceFactory; import org.opensearch.index.seqno.RetentionLeases; import org.opensearch.index.translog.TranslogDeletionPolicy; import org.opensearch.index.translog.TranslogDeletionPolicyFactory; @@ -40,7 +41,7 @@ public void testCreateEngineConfigFromFactory() { .build(); List plugins = Collections.singletonList(new FooEnginePlugin()); IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", meta.getSettings()); - EngineConfigFactory factory = new EngineConfigFactory(plugins, indexSettings); + EngineConfigFactory factory = new EngineConfigFactory(plugins, indexSettings, null); EngineConfig config = factory.newEngineConfig( null, @@ -81,7 +82,19 @@ public void testCreateEngineConfigFromFactoryMultipleCodecServiceIllegalStateExc List plugins = Arrays.asList(new FooEnginePlugin(), new BarEnginePlugin()); IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", meta.getSettings()); - expectThrows(IllegalStateException.class, () -> new EngineConfigFactory(plugins, indexSettings)); + expectThrows(IllegalStateException.class, () -> new EngineConfigFactory(plugins, indexSettings, null)); + } + + public void testCreateEngineConfigFromFactoryMultipleCodecServiceAndFactoryIllegalStateException() { + IndexMetadata meta = IndexMetadata.builder("test") + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .build(); + List plugins = Arrays.asList(new FooEnginePlugin(), new BakEnginePlugin()); + IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", meta.getSettings()); + + expectThrows(IllegalStateException.class, () -> new EngineConfigFactory(plugins, indexSettings, null)); } public void testCreateEngineConfigFromFactoryMultipleCustomTranslogDeletionPolicyFactoryIllegalStateException() { @@ -93,7 +106,44 @@ public void testCreateEngineConfigFromFactoryMultipleCustomTranslogDeletionPolic List plugins = Arrays.asList(new FooEnginePlugin(), new BazEnginePlugin()); IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", meta.getSettings()); - expectThrows(IllegalStateException.class, () -> new EngineConfigFactory(plugins, indexSettings)); + expectThrows(IllegalStateException.class, () -> new EngineConfigFactory(plugins, indexSettings, null)); + } + + public void testCreateCodecServiceFromFactory() { + IndexMetadata meta = IndexMetadata.builder("test") + .settings(settings(Version.CURRENT)) + .numberOfShards(1) + .numberOfReplicas(1) + .build(); + List plugins = Arrays.asList(new BakEnginePlugin()); + IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", meta.getSettings()); + + EngineConfigFactory factory = new EngineConfigFactory(plugins, indexSettings, null); + EngineConfig config = factory.newEngineConfig( + null, + null, + indexSettings, + null, + null, + null, + null, + null, + null, + null, + null, + null, + null, + TimeValue.timeValueMinutes(5), + null, + null, + null, + null, + null, + () -> new RetentionLeases(0, 0, Collections.emptyList()), + null, + null + ); + assertNotNull(config.getCodec()); } private static class FooEnginePlugin extends Plugin implements EnginePlugin { @@ -125,6 +175,18 @@ public Optional getCustomCodecService(IndexSettings indexSettings) } } + private static class BakEnginePlugin extends Plugin implements EnginePlugin { + @Override + public Optional getEngineFactory(final IndexSettings indexSettings) { + return Optional.empty(); + } + + @Override + public Optional getCustomCodecServiceFactory() { + return Optional.of(config -> new CodecService(config.getMapperService(), LogManager.getLogger(getClass()))); + } + } + private static class BazEnginePlugin extends Plugin implements EnginePlugin { @Override public Optional getEngineFactory(final IndexSettings indexSettings) {