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.

Sign up for GitHub

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

Jump to bottom

MapperService has to be passed in as null for EnginePlugins CodecService constructor #2177

Merged
merged 5 commits into from
Mar 8, 2022
Merged

MapperService has to be passed in as null for EnginePlugins CodecService constructor #2177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
45665db
MapperService has to be passed in as null for EnginePlugins CodecServ…
reta Feb 17, 2022
17c0abb
Addressing code review comments
reta Mar 4, 2022
5c136be
Delayed CodecService instantiation up to the shard initialization
reta Mar 4, 2022
eeeab29
Added logger (associated with shard) to CodecServiceConfig
reta Mar 7, 2022
5379899
Refactored the EngineConfigFactory / IndexShard instantiation of the …
reta Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* 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.Logger;
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;
Copy link
Member

Choose a reason for hiding this comment

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

If we are passing index settings to the getCustomCodecServiceFactory, do we need to include them in CodecServiceConfig?

Im just wondering if we should steer plugin developers to make decisions based on index settings in getCustomCodecServiceFactory instead of in createCodecService. Would this limit functionality? Does it really matter where decisions on index settings are being made?

I think I am leaning towards limiting it to getCustomCodecServiceFactory. What are your thoughts @nknize @reta?

Copy link
Collaborator Author

@reta reta Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We could, I believe the presence of the index settings could be beneficial, otherwise would leave that up to implementors to pass the index settings from EnginePlugin to CodecService instance if needed

private final MapperService mapperService;
private final Logger logger;

public CodecServiceConfig(IndexSettings indexSettings, @Nullable MapperService mapperService, @Nullable Logger logger) {
this.indexSettings = Objects.requireNonNull(indexSettings);
this.mapperService = mapperService;
this.logger = logger;
}

public IndexSettings getIndexSettings() {
return indexSettings;
}

@Nullable
public MapperService getMapperService() {
return mapperService;
}

@Nullable
public Logger getLogger() {
return logger;
}
}
22 changes: 22 additions & 0 deletions server/src/main/java/org/opensearch/index/codec/CodecServiceFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
59 changes: 53 additions & 6 deletions server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,21 @@

package org.opensearch.index.engine;

import org.apache.logging.log4j.Logger;
import org.apache.lucene.analysis.Analyzer;
import org.apache.lucene.index.MergePolicy;
import org.apache.lucene.search.QueryCache;
import org.apache.lucene.search.QueryCachingPolicy;
import org.apache.lucene.search.ReferenceManager;
import org.apache.lucene.search.Sort;
import org.apache.lucene.search.similarities.Similarity;
import org.opensearch.common.Nullable;
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;
Expand All @@ -39,7 +44,7 @@
* A factory to create an EngineConfig based on custom plugin overrides
*/
public class EngineConfigFactory {
private final CodecService codecService;
private final CodecServiceFactory codecServiceFactory;
private final TranslogDeletionPolicyFactory translogDeletionPolicyFactory;

/** default ctor primarily used for tests without plugins */
Expand All @@ -58,14 +63,16 @@ public EngineConfigFactory(PluginsService pluginsService, IndexSettings idxSetti
EngineConfigFactory(Collection<EnginePlugin> enginePlugins, IndexSettings idxSettings) {
Optional<CodecService> codecService = Optional.empty();
String codecServiceOverridingPlugin = null;
Optional<CodecServiceFactory> codecServiceFactory = Optional.empty();
String codecServiceFactoryOverridingPlugin = null;
Optional<TranslogDeletionPolicyFactory> translogDeletionPolicyFactory = Optional.empty();
String translogDeletionPolicyOverridingPlugin = null;
for (EnginePlugin enginePlugin : enginePlugins) {
// get overriding codec service from EnginePlugin
if (codecService.isPresent() == false) {
codecService = enginePlugin.getCustomCodecService(idxSettings);
codecServiceOverridingPlugin = enginePlugin.getClass().getName();
} else {
} else if (enginePlugin.getCustomCodecService(idxSettings).isPresent()) {
Copy link
Collaborator Author

@reta reta Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This seems to be a bug: the IllegalStateException is going to be thrown even if enginePlugin does not provide any codec service (returns Optional.empty).

Copy link
Member

Choose a reason for hiding this comment

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

Seems bad. Has this case just never been hit because in practice there is only ever 1 engine plugin or because the last plugin in the list is the one to override the codec service?

Copy link
Collaborator Author

@reta reta Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hard to say, but I would guess that only handful of engine plugins do provide codec service (and probably there is usually only 1 as you mentioned).

Copy link
Member

Choose a reason for hiding this comment

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

Right, and I believe this change was introduced in 1.2 as well

throw new IllegalStateException(
"existing codec service already overridden in: "
+ codecServiceOverridingPlugin
Expand All @@ -76,20 +83,45 @@ public EngineConfigFactory(PluginsService pluginsService, IndexSettings idxSetti
if (translogDeletionPolicyFactory.isPresent() == false) {
translogDeletionPolicyFactory = enginePlugin.getCustomTranslogDeletionPolicyFactory();
translogDeletionPolicyOverridingPlugin = enginePlugin.getClass().getName();
} else {
} else if (enginePlugin.getCustomTranslogDeletionPolicyFactory().isPresent()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same here

throw new IllegalStateException(
"existing TranslogDeletionPolicyFactory is already overridden in: "
+ translogDeletionPolicyOverridingPlugin
+ " attempting to override again by: "
+ enginePlugin.getClass().getName()
);
}
// get overriding CodecServiceFactory from EnginePlugin
if (codecServiceFactory.isPresent() == false) {
codecServiceFactory = enginePlugin.getCustomCodecServiceFactory(idxSettings);
codecServiceFactoryOverridingPlugin = enginePlugin.getClass().getName();
} else if (enginePlugin.getCustomCodecServiceFactory(idxSettings).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);

final CodecService instance = codecService.orElse(null);
this.codecServiceFactory = (instance != null) ? (config) -> instance : codecServiceFactory.orElse(null);
this.translogDeletionPolicyFactory = translogDeletionPolicyFactory.orElse((idxs, rtls) -> null);
}

/** Instantiates a new EngineConfig from the provided custom overrides */
Copy link
Member

Choose a reason for hiding this comment

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

Why cant we just delete this method and go with 179? This method looks to only be used IndexShard. Is it possible for plugin developers to access EngineConfigFactory.java?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a public method which could be used (by plugins fe) so we should not break them

/**
* Instantiates a new EngineConfig from the provided custom overrides
*/
public EngineConfig newEngineConfig(
ShardId shardId,
ThreadPool threadPool,
Expand All @@ -114,6 +146,10 @@ public EngineConfig newEngineConfig(
LongSupplier primaryTermSupplier,
EngineConfig.TombstoneDocSupplier tombstoneDocSupplier
) {
CodecService codecServiceToUse = codecService;
if (codecService == null && this.codecServiceFactory != null) {
codecServiceToUse = newCodecServiceOrDefault(indexSettings, null, null, null);
}

return new EngineConfig(
shardId,
Expand All @@ -124,7 +160,7 @@ public EngineConfig newEngineConfig(
mergePolicy,
analyzer,
similarity,
this.codecService != null ? this.codecService : codecService,
codecServiceToUse,
eventListener,
queryCache,
queryCachingPolicy,
Expand All @@ -141,4 +177,15 @@ public EngineConfig newEngineConfig(
tombstoneDocSupplier
);
}

public CodecService newCodecServiceOrDefault(
IndexSettings indexSettings,
@Nullable MapperService mapperService,
Logger logger,
CodecService defaultCodecService
) {
return this.codecServiceFactory != null
? this.codecServiceFactory.createCodecService(new CodecServiceConfig(indexSettings, mapperService, logger))
: defaultCodecService;
}
}
3 changes: 2 additions & 1 deletion server/src/main/java/org/opensearch/index/shard/IndexShard.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3155,6 +3155,7 @@ private EngineConfig newEngineConfig(LongSupplier globalCheckpointSupplier) {
this.warmer.warm(reader);
}
};

return this.engineConfigFactory.newEngineConfig(
shardId,
threadPool,
Expand All @@ -3164,7 +3165,7 @@ private EngineConfig newEngineConfig(LongSupplier globalCheckpointSupplier) {
indexSettings.getMergePolicy(),
mapperService != null ? mapperService.indexAnalyzer() : null,
similarityService.similarity(mapperService),
codecService,
engineConfigFactory.newCodecServiceOrDefault(indexSettings, mapperService, logger, codecService),
shardEventListener,
indexCache != null ? indexCache.query() : null,
cachingPolicy,
Expand Down
16 changes: 16 additions & 0 deletions server/src/main/java/org/opensearch/plugins/EnginePlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<CodecService> 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<CodecServiceFactory> getCustomCodecServiceFactory(IndexSettings indexSettings) {
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}
Expand Down
62 changes: 62 additions & 0 deletions server/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -84,6 +85,18 @@ public void testCreateEngineConfigFromFactoryMultipleCodecServiceIllegalStateExc
expectThrows(IllegalStateException.class, () -> new EngineConfigFactory(plugins, indexSettings));
}

public void testCreateEngineConfigFromFactoryMultipleCodecServiceAndFactoryIllegalStateException() {
IndexMetadata meta = IndexMetadata.builder("test")
.settings(settings(Version.CURRENT))
.numberOfShards(1)
.numberOfReplicas(1)
.build();
List<EnginePlugin> plugins = Arrays.asList(new FooEnginePlugin(), new BakEnginePlugin());
IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", meta.getSettings());

expectThrows(IllegalStateException.class, () -> new EngineConfigFactory(plugins, indexSettings));
}

public void testCreateEngineConfigFromFactoryMultipleCustomTranslogDeletionPolicyFactoryIllegalStateException() {
IndexMetadata meta = IndexMetadata.builder("test")
.settings(settings(Version.CURRENT))
Expand All @@ -96,6 +109,43 @@ public void testCreateEngineConfigFromFactoryMultipleCustomTranslogDeletionPolic
expectThrows(IllegalStateException.class, () -> new EngineConfigFactory(plugins, indexSettings));
}

public void testCreateCodecServiceFromFactory() {
IndexMetadata meta = IndexMetadata.builder("test")
.settings(settings(Version.CURRENT))
.numberOfShards(1)
.numberOfReplicas(1)
.build();
List<EnginePlugin> plugins = Arrays.asList(new BakEnginePlugin());
IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", meta.getSettings());

EngineConfigFactory factory = new EngineConfigFactory(plugins, indexSettings);
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 {
@Override
public Optional<EngineFactory> getEngineFactory(final IndexSettings indexSettings) {
Expand Down Expand Up @@ -125,6 +175,18 @@ public Optional<CodecService> getCustomCodecService(IndexSettings indexSettings)
}
}

private static class BakEnginePlugin extends Plugin implements EnginePlugin {
@Override
public Optional<EngineFactory> getEngineFactory(final IndexSettings indexSettings) {
return Optional.empty();
}

@Override
public Optional<CodecServiceFactory> getCustomCodecServiceFactory(IndexSettings indexSettings) {
return Optional.of(config -> new CodecService(config.getMapperService(), LogManager.getLogger(getClass())));
}
}

private static class BazEnginePlugin extends Plugin implements EnginePlugin {
@Override
public Optional<EngineFactory> getEngineFactory(final IndexSettings indexSettings) {
Expand Down