Exclusively use Factory to build CodecService

Changes EngineFactoryConfig constructor to always build non-null CodecServiceFactory. Then, use the CodecServiceFactory in IndexShard to build CodecService in one place. Signed-off-by: John Mazanec <[email protected]>
jmazanec15 · Mar 4, 2022 · 165f50a · 165f50a
1 parent 5c136be
commit 165f50a
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 67 deletions.
diff --git a/server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java b/server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java
@@ -8,6 +8,7 @@
 
 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;
@@ -20,10 +21,12 @@
 public final class CodecServiceConfig {
     private final IndexSettings indexSettings;
     private final MapperService mapperService;
+    private final Logger logger;
 
-    public CodecServiceConfig(IndexSettings indexSettings, @Nullable MapperService mapperService) {
+    public CodecServiceConfig(IndexSettings indexSettings, @Nullable MapperService mapperService, @Nullable Logger logger) {
         this.indexSettings = Objects.requireNonNull(indexSettings);
         this.mapperService = mapperService;
+        this.logger = logger;
     }
 
     public IndexSettings getIndexSettings() {
@@ -34,4 +37,9 @@ public IndexSettings getIndexSettings() {
     public MapperService getMapperService() {
         return mapperService;
     }
+
+    @Nullable
+    public Logger getLogger() {
+        return logger;
+    }
 }
diff --git a/server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java b/server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java
@@ -18,9 +18,7 @@
 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;
@@ -112,65 +110,20 @@ public EngineConfigFactory(PluginsService pluginsService, IndexSettings idxSetti
             );
         }
 
-        final CodecService instance = codecService.orElse(null);
-        this.codecServiceFactory = (instance != null) ? (config) -> instance : codecServiceFactory.orElse(null);
+        if (codecService.isPresent()) {
+            final CodecService instance = codecService.get();
+            this.codecServiceFactory = (config) -> instance;
+        } else if (codecServiceFactory.isPresent()) {
+            this.codecServiceFactory = codecServiceFactory.get();
+        } else {
+            this.codecServiceFactory = (config) -> new CodecService(config.getMapperService(), config.getLogger());
+        }
+
         this.translogDeletionPolicyFactory = translogDeletionPolicyFactory.orElse((idxs, rtls) -> null);
     }
 
-    /**
-     * 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,
-        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
-    ) {
-        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
-        );
+    public CodecServiceFactory getCodecServiceFactory() {
+        return codecServiceFactory;
     }
 
     /** Instantiates a new EngineConfig from the provided custom overrides */
@@ -196,8 +149,7 @@ public EngineConfig newEngineConfig(
         LongSupplier globalCheckpointSupplier,
         Supplier<RetentionLeases> retentionLeasesSupplier,
         LongSupplier primaryTermSupplier,
-        EngineConfig.TombstoneDocSupplier tombstoneDocSupplier,
-        MapperService mapperService
+        EngineConfig.TombstoneDocSupplier tombstoneDocSupplier
     ) {
 
         return new EngineConfig(
@@ -209,9 +161,7 @@ public EngineConfig newEngineConfig(
             mergePolicy,
             analyzer,
             similarity,
-            this.codecServiceFactory != null
-                ? this.codecServiceFactory.createCodecService(new CodecServiceConfig(indexSettings, mapperService))
-                : codecService,
+            codecService,
             eventListener,
             queryCache,
             queryCachingPolicy,

diff --git a/server/src/main/java/org/opensearch/index/shard/IndexShard.java b/server/src/main/java/org/opensearch/index/shard/IndexShard.java
@@ -101,6 +101,7 @@
 import org.opensearch.index.cache.bitset.ShardBitsetFilterCache;
 import org.opensearch.index.cache.request.ShardRequestCache;
 import org.opensearch.index.codec.CodecService;
+import org.opensearch.index.codec.CodecServiceConfig;
 import org.opensearch.index.engine.CommitStats;
 import org.opensearch.index.engine.Engine;
 import org.opensearch.index.engine.Engine.GetResult;
@@ -321,12 +322,13 @@ public IndexShard(
         assert shardRouting.initializing();
         this.shardRouting = shardRouting;
         final Settings settings = indexSettings.getSettings();
-        this.codecService = new CodecService(mapperService, logger);
         this.warmer = warmer;
         this.similarityService = similarityService;
         Objects.requireNonNull(store, "Store must be provided to the index shard");
         this.engineFactory = Objects.requireNonNull(engineFactory);
         this.engineConfigFactory = Objects.requireNonNull(engineConfigFactory);
+        this.codecService = this.engineConfigFactory.getCodecServiceFactory().createCodecService(
+            new CodecServiceConfig(indexSettings, mapperService, logger));
         this.store = store;
         this.indexSortSupplier = indexSortSupplier;
         this.indexEventListener = indexEventListener;
@@ -3177,8 +3179,7 @@ private EngineConfig newEngineConfig(LongSupplier globalCheckpointSupplier) {
             globalCheckpointSupplier,
             replicationTracker::getRetentionLeases,
             () -> getOperationPrimaryTerm(),
-            tombstoneDocSupplier(),
-            mapperService
+            tombstoneDocSupplier()
         );
     }