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 2 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
37 changes: 37 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,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;
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;

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;
}
}
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);
}
50 changes: 44 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 @@ -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;
Expand All @@ -44,28 +47,37 @@ 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<EnginePlugin> enginePlugins, IndexSettings idxSettings) {
EngineConfigFactory(Collection<EnginePlugin> enginePlugins, IndexSettings idxSettings, MapperService mapperService) {
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,16 +88,42 @@ 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);

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);
}

Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/opensearch/indices/IndicesService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

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

When would IndexService be null? If it is should it fail?

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 good question - I cannot say exactly, but since indices is just map, the get may return null, better be ready for it.

Copy link
Member

Choose a reason for hiding this comment

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

Im wondering if this will always be null?

getEngineConfigFactory gets called in createIndexService , which gets called in createIndex, but indices is not updated until after.

The mapper service will get created in IndexService's constructor.

The CodecService that is used when EnginePlugins are not used is actually created in IndexShard's constructor. Eventually, there is this somewhat hack in EngineConfigFactory.newEngineConfig where if there is a CodecService set in the EngineConfigFactory, it is used, otherwise the passed in one is used.

But I believe that the mapper service will not have been created at this point.

Copy link
Member

Choose a reason for hiding this comment

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

I think my opinion is that we should only create a codec service in one place and it should be here: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/index/shard/IndexShard.java#L324. We should pass in the CodecService factory there and get rid of the codec service in EngineConfigFactory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very good point, looking into it now, thank you!

Copy link
Member

Choose a reason for hiding this comment

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

Could we do something like this: jmazanec15@165f50a? This way, we can exclusively use CodecServiceFactory to build codec and not change any function interfaces?

Alternatively, I thought about building CodecService directly in newEngineConfig. In this case, we would switch from passing in CodecService directly to passing in CodecServiceConfig. But, I thought this might cause us to create more CodecServices than we need to.

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.

@jmazanec15 hm ... to me, the logger service does not belong here, may be if you share use case, we could think about it?

Copy link
Member

Choose a reason for hiding this comment

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

@reta Logger is one of the constructor arguments to CodecService. I think the CodecServiceConfig will need to have it to build CodecService - similar to MapperService. That way, we can use CodecServiceFactory to build default CodecService.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jmazanec15 I should have looked it :) Thank you, will do the change shortly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, thank @jmazanec15 !

return new EngineConfigFactory(this.pluginsService, idxSettings, (indexService != null) ? indexService.mapperService() : null);
}

private EngineFactory getEngineFactory(final IndexSettings idxSettings) {
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
68 changes: 65 additions & 3 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 All @@ -40,7 +41,7 @@ public void testCreateEngineConfigFromFactory() {
.build();
List<EnginePlugin> 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,
Expand Down Expand Up @@ -81,7 +82,19 @@ public void testCreateEngineConfigFromFactoryMultipleCodecServiceIllegalStateExc
List<EnginePlugin> 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<EnginePlugin> 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() {
Expand All @@ -93,7 +106,44 @@ public void testCreateEngineConfigFromFactoryMultipleCustomTranslogDeletionPolic
List<EnginePlugin> 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<EnginePlugin> 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 {
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