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

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 4 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
44 changes: 44 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,44 @@
/*
* 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, Logger logger) {
this.indexSettings = Objects.requireNonNull(indexSettings);
this.mapperService = mapperService;
this.logger = logger;
}

public IndexSettings getIndexSettings() {
return indexSettings;
}

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

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);
}
102 changes: 96 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,6 +8,7 @@

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;
Expand All @@ -18,6 +19,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 @@ -39,7 +43,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 +62,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 +82,47 @@ 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
* @deprecated please use overloaded {@code newEngineConfig} with {@link MapperService}
*/
@Deprecated
public EngineConfig newEngineConfig(
ShardId shardId,
ThreadPool threadPool,
Expand All @@ -114,6 +147,61 @@ public EngineConfig newEngineConfig(
LongSupplier primaryTermSupplier,
EngineConfig.TombstoneDocSupplier tombstoneDocSupplier
) {
return newEngineConfig(
shardId,
threadPool,
indexSettings,
warmer,
store,
mergePolicy,
analyzer,
similarity,
codecService,
eventListener,
queryCache,
queryCachingPolicy,
translogConfig,
flushMergesAfter,
externalRefreshListener,
internalRefreshListener,
indexSort,
circuitBreakerService,
globalCheckpointSupplier,
retentionLeasesSupplier,
primaryTermSupplier,
tombstoneDocSupplier,
null, /* mapperService */
null /* logger */
);
}

/** Instantiates a new EngineConfig from the provided custom overrides */
public EngineConfig newEngineConfig(
ShardId shardId,
ThreadPool threadPool,
IndexSettings indexSettings,
Engine.Warmer warmer,
Store store,
MergePolicy mergePolicy,
Analyzer analyzer,
Similarity similarity,
CodecService codecService,
Engine.EventListener eventListener,
QueryCache queryCache,
QueryCachingPolicy queryCachingPolicy,
TranslogConfig translogConfig,
TimeValue flushMergesAfter,
List<ReferenceManager.RefreshListener> externalRefreshListener,
List<ReferenceManager.RefreshListener> internalRefreshListener,
Sort indexSort,
CircuitBreakerService circuitBreakerService,
LongSupplier globalCheckpointSupplier,
Supplier<RetentionLeases> retentionLeasesSupplier,
LongSupplier primaryTermSupplier,
EngineConfig.TombstoneDocSupplier tombstoneDocSupplier,
MapperService mapperService,
Logger logger
) {

return new EngineConfig(
shardId,
Expand All @@ -124,7 +212,9 @@ public EngineConfig newEngineConfig(
mergePolicy,
analyzer,
similarity,
this.codecService != null ? this.codecService : codecService,
this.codecServiceFactory != null
? this.codecServiceFactory.createCodecService(new CodecServiceConfig(indexSettings, mapperService, logger))
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 we should only build CodecService in one place and always use the factory. That way, we can avoid ternary statements like the above. For default CodecServiceFactory, we can set it like I did here.

Im somewhat conflicted on where to build CodecService. I think there are 2 options:

1. Call this.codecServiceFactory.createCodecService here and dont take in CodecService as arg to newEngineConfig

The pro's to this approach are that we can hide details about CodecService from all other parts of code - in IndexShard we dont need to do anything with codec service. Its all handled by EngineConfigFactory.

The cons are that a new CodecService would get created every time this method gets called. I'm not really sure if there will be side effects to this. In IndexShard, this gets called in resetEngineToGlobalCheckpoint and innerOpenEngineAndTranslog. One potential issue could arise if a user changes IndexSettings between these 2 operations and that impacts how the CodecServiceFactory builds the CodecService (maybe this is a feature not a bug?).

2. Call it in IndexShard's constructor by exposing getter on EngineConfigFactory's codecServiceFactory

Alternatively, we could create the codec service in IndexShards constructor. This is how it is being done now for the default. To do this, we would need to expose a getter method for EngineConfigFactory to get its codecServiceFactory and then call this.codecServiceFactory.createCodecService(). Then, we wouldnt change anything in EngineConfigFactory.newEngineConfig(). The getter, however, feels a little bit out of place here. I took this approach here.

I think I am leaning towards approach 2 as opposed to 1, to keep consistent with how its being done now.

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 think I got your idea, the challenge here is to preserve the semantics of the CodecService instantiation, I just pushed the solution which I believe is the best effort:

  • new method EngineConfigFactory::newCodecServiceOrDefault which basically either create a new CodecService instance from factory or uses the existing one
  • IndexShard::newEngineConfig uses EngineConfigFactory::newCodecServiceOrDefault to pass the CodecService instance to EngineConfigFactory::newEngineConfig
  • EngineConfigFactory::newEngineConfig has the fallback in case passed CodecService is null but there is CodecServiceFactory configured - the old way to create CodecService without MapperService and Logger

The APIs stay unchanged, 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.

That makes sense thanks!

: codecService,
eventListener,
queryCache,
queryCachingPolicy,
Expand Down
4 changes: 3 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 @@ -3177,7 +3177,9 @@ private EngineConfig newEngineConfig(LongSupplier globalCheckpointSupplier) {
globalCheckpointSupplier,
replicationTracker::getRetentionLeases,
() -> getOperationPrimaryTerm(),
tombstoneDocSupplier()
tombstoneDocSupplier(),
mapperService,
logger
);
}

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