Introduce AdditionalCodecs and EnginePlugin::getAdditionalCodecs hook to allow additional Codec registration#20411
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughAdds a plugin-extensible codec registry: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as EnginePlugin
participant Factory as EngineConfigFactory
participant Config as CodecServiceConfig
participant Service as CodecService
participant Registry as CodecRegistry
Plugin->>Factory: getAdditionalCodecs(indexSettings)
Factory->>Factory: collect registries from plugins
Factory->>Config: create CodecServiceConfig(..., registries)
Factory->>Service: new CodecService(mapperService, indexSettings, logger, registries)
Service->>Registry: registry.getCodecs(mapperService, indexSettings, defaultCodec)
Registry-->>Service: Map<String, Codec>
Service->>Service: merge registry codecs into built-ins
Service->>Registry: registry.onConflict(name, oldCodec, newCodec)
Service-->>Factory: initialized CodecService with merged codec set
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
251c134 to
b453b61
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/src/test/java/org/opensearch/index/translog/TranslogOperationHelperTests.java (1)
18-33: Fix compilation: missingjava.util.Listimport after introducingList.of().
This file now callsList.of()(Line 82) but doesn’t importjava.util.List.Proposed fix
@@ import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.List; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when;Also applies to: 75-85
server/src/test/java/org/opensearch/index/engine/NRTReplicationReaderManagerTests.java (1)
26-29: Fix compilation:List.of()used without importingjava.util.List(preferemptyList()already imported).Proposed fix (minimal: reuse existing import)
@@ - .codecService(new CodecService(null, defaultSettings, logger, List.of())) + .codecService(new CodecService(null, defaultSettings, logger, emptyList()))Also applies to: 38-62
server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java (1)
8810-8834: Use the sameIndexSettingsforCodecServiceas theEngineConfigbeing built.
EngineConfig.Builderis configured with.indexSettings(indexSettings), butCodecServiceis created withconfig.getIndexSettings()(potentially different instance/settings), which can make the test setup internally inconsistent.Proposed fix
- .codecService(new CodecService(null, config.getIndexSettings(), logger, List.of())) + .codecService(new CodecService(null, indexSettings, logger, List.of()))
🤖 Fix all issues with AI agents
In @server/src/main/java/org/opensearch/index/codec/CodecService.java:
- Around line 112-133: defaultCodec() can be invoked by
CodecRegistry.getCodecs() while this.codecs is still null, causing NPE; fix by
evaluating the default codec once before iterating registries and pass a
supplier that returns that captured instance (e.g., compute Codec
capturedDefault = defaultCodec(); then call registry.getCodecs(mapperService,
indexSettings, () -> capturedDefault) inside the for-loop), ensuring you
reference the capturedDefault instead of this::defaultCodec so registries never
call into an uninitialized this.codecs.
In @server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java:
- Around line 28-40: Make the registries field null-safe and immutable by
defensive copying in the CodecServiceConfig constructor: when assigning
this.registries, replace the direct assignment with an unmodifiable copy (e.g.,
Collections.unmodifiableList(new
ArrayList<>(Objects.requireNonNullElse(registries, Collections.emptyList()))));
and ensure any getter that returns registries (the method referencing
registries) simply returns the unmodifiable field rather than the original list
reference so callers cannot mutate the internal list.
In
@server/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.java:
- Line 186: The test uses List.of() in CodecService constructor calls (in
TranslogLeafReaderTests near the other occurrences) but the java.util.List
import is missing; add an import for java.util.List (or replace List.of() with a
fully-qualified java.util.List.of(...) call) so the CodecService(...) lines
compile — update the occurrences around the CodecService constructor usages (the
instances at the same place as the earlier fix and the additional one near the
second mentioned occurrence).
- Line 78: The test class TranslogLeafReaderTests uses List.of() (e.g., in the
CodecService constructor call and other places) but does not import
java.util.List; add the missing import "import java.util.List;" to the imports
section so the references to List.of() compile (fix occurrences around the
CodecService usage and the other two List.of() calls).
In
@server/src/test/java/org/opensearch/indices/replication/OngoingSegmentReplicationsTests.java:
- Line 76: The compilation fails because java.util.List is not imported while
using List.of() in the test; add the missing import by adding import
java.util.List; to the test file's import section so the reference to List.of()
in the CodecService instantiation (CodecService codecService = new
CodecService(..., List.of())) resolves.
🧹 Nitpick comments (4)
server/src/test/java/org/opensearch/gateway/PrimaryShardBatchAllocatorTests.java (1)
393-415: Avoid hard-coded"default"and reduce duplication in codec lookup (test cleanup).Not a blocker, but using
CodecService.DEFAULT_CODECavoids string drift, and a tiny helper keeps the test intent clearer.Possible refactor
@@ public TestBatchAllocator addData(DiscoveryNode node, String allocationId, boolean primary) { Settings nodeSettings = Settings.builder().put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()).build(); IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", nodeSettings); + final String codecName = new CodecService(null, indexSettings, null, List.of()) + .codec(CodecService.DEFAULT_CODEC) + .getName(); return addData( node, allocationId, primary, - ReplicationCheckpoint.empty(shardId, new CodecService(null, indexSettings, null, List.of()).codec("default").getName()), + ReplicationCheckpoint.empty(shardId, codecName), null ); } @@ public TestBatchAllocator addData(DiscoveryNode node, String allocationId, boolean primary, @Nullable Exception storeException) { Settings nodeSettings = Settings.builder().put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()).build(); IndexSettings indexSettings = IndexSettingsModule.newIndexSettings("test", nodeSettings); + final String codecName = new CodecService(null, indexSettings, null, List.of()) + .codec(CodecService.DEFAULT_CODEC) + .getName(); return addData( node, allocationId, primary, - ReplicationCheckpoint.empty(shardId, new CodecService(null, indexSettings, null, List.of()).codec("default").getName()), + ReplicationCheckpoint.empty(shardId, codecName), storeException ); }server/src/main/java/org/opensearch/plugins/EnginePlugin.java (1)
92-102: Well-designed extension point for custom codec registration.The new
getAdditionalCodecsmethod provides a clean plugin extension point that follows the established pattern in this interface. The Javadoc clearly explains the use case for codecs requiring complex instantiation logic that cannot be registered through Lucene's service loader.A few optional suggestions to consider:
For consistency with
getEngineFactory(), consider documenting when this method is invoked (e.g., "When an index is created this method is invoked for each engine plugin").Consider adding a
@since 3.5.0tag for API discoverability.📝 Optional: Enhanced Javadoc for consistency
/** + * When an index is created this method is invoked for each engine plugin. * Apache Lucene uses service loader to discover available {@link org.apache.lucene.codecs.Codec}, * however sometimes custom {@link org.apache.lucene.codecs.Codec} implementations do require * complex instantiation logic and could not be registered through service loader. The * {@link CodecRegistry} is designated as a mechanism to contribute additional * {@link org.apache.lucene.codecs.Codec} that require non-trivial instantiation * logic. + * + * @param indexSettings the index settings + * @return an optional codec registry + * @since 3.5.0 */server/src/main/java/org/opensearch/index/codec/CodecRegistry.java (1)
47-64: Incomplete Javadoc parameter descriptions foronConflictmethod.The
@paramand@returntags at lines 57-60 are empty. Consider adding descriptions for clarity:- * @param name - * @param oldCodec - * @param newCodec - * @return + * @param name the codec name causing the conflict + * @param oldCodec the previously registered codec + * @param newCodec the new codec being registered + * @return the codec to register, or {@code null} to remove ittest/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java (1)
264-264: Consider reusingCodecServicefrom the source config incopy()methods.The
copy()methods create a freshCodecServiceinstance instead of reusingconfig.getCodecService(). If the intent is to preserve the original configuration's codec behavior (including any plugin-provided codecs), consider:-.codecService(new CodecService(null, config.getIndexSettings(), logger, List.of())) +.codecService(config.getCodecService())However, if the intent is to reset to a baseline codec service for isolation in tests, the current approach is acceptable.
Also applies to: 290-290, 316-316
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
server/src/internalClusterTest/java/org/opensearch/indices/IndexingMemoryControllerIT.javaserver/src/main/java/org/opensearch/index/codec/CodecRegistry.javaserver/src/main/java/org/opensearch/index/codec/CodecService.javaserver/src/main/java/org/opensearch/index/codec/CodecServiceConfig.javaserver/src/main/java/org/opensearch/index/engine/EngineConfigFactory.javaserver/src/main/java/org/opensearch/index/shard/IndexShard.javaserver/src/main/java/org/opensearch/plugins/EnginePlugin.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardAllocatorTests.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardBatchAllocatorTests.javaserver/src/test/java/org/opensearch/index/codec/CodecTests.javaserver/src/test/java/org/opensearch/index/engine/CriteriaBasedCompositeIndexWriterBaseTests.javaserver/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.javaserver/src/test/java/org/opensearch/index/engine/InternalEngineTests.javaserver/src/test/java/org/opensearch/index/engine/LeafSorterOptimizationTests.javaserver/src/test/java/org/opensearch/index/engine/NRTReplicationReaderManagerTests.javaserver/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.javaserver/src/test/java/org/opensearch/index/shard/IndexShardTests.javaserver/src/test/java/org/opensearch/index/shard/RefreshListenersTests.javaserver/src/test/java/org/opensearch/index/translog/TranslogOperationHelperTests.javaserver/src/test/java/org/opensearch/indices/IndexingMemoryControllerTests.javaserver/src/test/java/org/opensearch/indices/replication/OngoingSegmentReplicationsTests.javatest/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-02T22:44:14.799Z
Learnt from: prudhvigodithi
Repo: opensearch-project/OpenSearch PR: 20112
File: server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java:73-81
Timestamp: 2025-12-02T22:44:14.799Z
Learning: In OpenSearch integration tests extending OpenSearchIntegTestCase, using `LuceneTestCase.SuppressCodecs("*")` triggers special handling that selects a random production codec from the CODECS array, while `SuppressCodecs("Asserting")` or other specific codec suppressions still allow Lucene's default codec randomization which may include the asserting codec. Use `SuppressCodecs("*")` when you need to completely avoid asserting codecs (e.g., for cross-thread StoredFieldsReader usage) while maintaining production codec test coverage.
Applied to files:
server/src/test/java/org/opensearch/index/translog/TranslogOperationHelperTests.javaserver/src/test/java/org/opensearch/indices/IndexingMemoryControllerTests.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardBatchAllocatorTests.javaserver/src/main/java/org/opensearch/plugins/EnginePlugin.javaserver/src/test/java/org/opensearch/index/shard/IndexShardTests.javaserver/src/test/java/org/opensearch/index/codec/CodecTests.javatest/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.javaserver/src/test/java/org/opensearch/indices/replication/OngoingSegmentReplicationsTests.javaserver/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.javaserver/src/test/java/org/opensearch/index/engine/InternalEngineTests.javaserver/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardAllocatorTests.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: Mend Security Check
🔇 Additional comments (28)
server/src/main/java/org/opensearch/index/shard/IndexShard.java (1)
439-452: Looks good—please sanity-check factory behavior (precedence + per-shard cost).Moving codecService creation behind
EngineConfigFactory.newDefaultCodecService(...)is a nice consolidation point for registry support. Only concern: ensure the factory doesn’t (a) recompute plugin registries per shard creation unnecessarily, and (b) preserves existing precedence rules (e.g., custom codec service factory vs defaults).server/src/test/java/org/opensearch/indices/IndexingMemoryControllerTests.java (1)
411-411: LGTM!The
CodecServiceconstructor call is correctly updated to include an empty registry list viaList.of(), aligning with the new four-parameter constructor signature. TheListimport is already present at line 58.server/src/test/java/org/opensearch/index/engine/LeafSorterOptimizationTests.java (2)
31-31: LGTM!The
java.util.Listimport is correctly added to support theList.of()calls in theCodecServiceconstructor invocations throughout the test file.
62-62: LGTM!All four
CodecServiceconstructor calls are consistently updated to include the empty registry list parameterList.of(), correctly aligning with the new API signature.Also applies to: 120-120, 182-182, 259-259
server/src/internalClusterTest/java/org/opensearch/indices/IndexingMemoryControllerIT.java (1)
92-92: LGTM!The
CodecServiceconstructor call is correctly updated to include an empty registry list. TheListimport is already present at line 51.server/src/main/java/org/opensearch/plugins/EnginePlugin.java (1)
36-36: LGTM!The
CodecRegistryimport is correctly added to support the new method signature.server/src/test/java/org/opensearch/index/engine/CriteriaBasedCompositeIndexWriterBaseTests.java (1)
299-299: LGTM!The
CodecServiceconstructor call is correctly updated to include an empty registry list. TheListimport is already present at line 85.server/src/test/java/org/opensearch/gateway/PrimaryShardAllocatorTests.java (1)
815-815: LGTM!Both
CodecServiceconstructor calls in theTestAllocator.addData()helper methods are correctly updated to include the empty registry list parameter. TheListimport is already present at line 79.Also applies to: 827-827
server/src/test/java/org/opensearch/index/shard/RefreshListenersTests.java (1)
155-177: Constructor update looks correct; empty registries list is fine.
new CodecService(null, indexSettings, logger, List.of())keeps prior test behavior while matching the new API.server/src/test/java/org/opensearch/index/shard/IndexShardTests.java (1)
5140-5164: LGTM: aligns test EngineConfig construction with new CodecService signature.
PassingList.of()for registries preserves existing behavior.server/src/test/java/org/opensearch/index/codec/CodecTests.java (1)
60-63: Good test updates for new constructor; keep@SuppressCodecs("*")here.
java.util.Listimport andList.of()usage is consistent across the suite.@SuppressCodecs("*")remains the right choice to avoid asserting codecs while still testing production codecs. Based on learnings, …Also applies to: 173-175, 189-203, 205-230
server/src/main/java/org/opensearch/index/codec/CodecRegistry.java (1)
34-45: LGTM!The interface design is well-thought-out. The
getCodecsmethod signature appropriately:
- Allows nullable
MapperServicefor cases where it's unavailable- Provides a
Supplier<Codec>for lazy access to the default codec (useful for wrapping scenarios)- Returns a
Map<String, Codec>for flexibility in contributing multiple codecstest/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java (2)
220-220: LGTM!Test setup correctly updated to use the new 4-argument
CodecServiceconstructor with an empty registries list, which is appropriate for test scaffolding without plugin-provided codecs.
953-953: LGTM!The
config()builder methods correctly instantiateCodecServicewith the new constructor signature, passing an empty registries list for test isolation.Also applies to: 999-999, 1036-1036
server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java (4)
61-61: LGTM!The
registriesfield is properly initialized:
- Uses
ArrayListfor accumulation during plugin iteration- Wrapped with
Collections.unmodifiableList()for immutability- Stored as a final field for thread-safety
Also applies to: 77-77, 137-137
120-122: LGTM!The registry collection logic correctly uses
ifPresentto handle theOptional<CodecRegistry>returned bygetAdditionalCodecs, cleanly adding registries from participating plugins.
214-216: LGTM!The new
newDefaultCodecServicemethod provides a clean factory method for creating aCodecServicewith the aggregated registries, which is now used byIndexShardfor the default codec service initialization path.
224-226: LGTM!The
newCodecServiceOrDefaultmethod correctly passes theregistriestoCodecServiceConfig, ensuring that plugin-provided codecs are available even when using a customCodecServiceFactory.server/src/main/java/org/opensearch/index/codec/CodecService.java (2)
72-78: LGTM!The deprecation of the 3-argument constructor with
@Deprecated(forRemoval = true)is appropriate. It correctly delegates to the new constructor with an empty registries list, maintaining backward compatibility during the migration period.
121-129: LGTM on conflict resolution logic.The three-way conflict handling is correctly implemented:
- If
resolved != null && resolved != existing: replace with new codec- If
resolved == null: remove the codec entirely- Otherwise (implicitly
resolved == existing): keep existing codecThe reference equality check (
resolved != existing) is intentional to detect whenonConflictreturns the exact same object.server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java (2)
3815-3820: Mechanical CodecService ctor update looks correct.Passing
List.of()keeps behavior identical (no additional registries) while compiling against the new constructor.
4316-4331: CodecService ctor updates in EngineConfig.Builder usages look correct.These are straightforward test wiring updates and should preserve prior behavior (no registries provided).
Also applies to: 4358-4374, 8010-8025
server/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.java (6)
12-13: LGTM!The new imports are all necessary:
CodecandSimpleTextCodecfor the codec registry test,CodecRegistryandMapperServicefor the plugin implementation,Mapfor the codec map return type, and Hamcrest matchers for the assertion.Also applies to: 18-18, 21-21, 37-37, 41-43
129-141: LGTM!The test correctly validates the new
getAdditionalCodecspathway by:
- Using
BazEnginePluginwhich implementsgetAdditionalCodecs- Creating a
CodecServiceviafactory.newDefaultCodecService- Asserting the custom "test" codec is available and is a
SimpleTextCodecThis provides good coverage for the new codec registry integration.
210-212: LGTM!The
CodecServiceconstructor call is correctly updated to includeList.of()for the new registries parameter, maintaining backward-compatible behavior.
227-229: LGTM!Correctly updated to pass an empty registry list to the new
CodecServiceconstructor.
238-248: LGTM!The
CodecServiceFactoryimplementation is properly updated to use the new constructor signature. The multi-line formatting improves readability.
261-274: LGTM!The
getAdditionalCodecsimplementation correctly returns aCodecRegistrythat provides aSimpleTextCodecunder the "test" key. This exercises the new plugin extension point effectively.Note that
BazEnginePluginnow has dual responsibility (translog deletion policy and additional codecs), which is appropriate for testing different aspects. The existing test at line 123 still correctly expects anIllegalStateExceptiondue to translog policy conflicts withFooEnginePlugin.
server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.java
Show resolved
Hide resolved
server/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.java
Show resolved
Hide resolved
server/src/test/java/org/opensearch/indices/replication/OngoingSegmentReplicationsTests.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
CHANGELOG.md (1)
15-15: Changelog entry is correct; consider naming “Lucene Codec implementations” explicitly for clarity.
This matches the PR intent and helps readers understand it’s about Lucene codecs (not generic codecs).server/src/main/java/org/opensearch/plugins/EnginePlugin.java (1)
35-102: Clarify composition/precedence ingetAdditionalCodecsJavadoc (and fix minor typos).Since multiple
EnginePlugins can contribute registries, it’d help to explicitly document:
- whether multiple non-empty returns are expected/combined
- where conflicts are resolved (e.g., via
CodecRegistry#onConflict)- whether this hook is ignored when a plugin provides a custom
CodecServiceFactory/CodecServiceAlso: “could not” → “could not”, and “additional Codecs”/pluralization tweaks.
server/src/test/java/org/opensearch/indices/IndexingMemoryControllerTests.java (1)
399-426: Avoid resetting codec registries to empty when cloningEngineConfigin tests.
configWithRefreshListener(...)rebuildsEngineConfigbut hard-codes an empty-registryCodecService. Prefer passing through the original codec service (or preserving its registries) to keep this test aligned with the new codec-registry plumbing.server/src/main/java/org/opensearch/index/codec/CodecRegistry.java (1)
47-64: Add descriptions to@paramand@returnJavadoc tags.The
onConflictmethod's Javadoc has empty@paramand@returntags. Consider adding brief descriptions for consistency with the rest of the documentation:/** * In case there is already registered {@code Codec} with the same name, allows to resolve * the conflict by: * - returning a {@code newCodec}, replaces the {@code oldCodec} * - returning a {@code oldCodec}, effectively ignores the {@code newCodec} * - returning a {@code null}, removes the {@code Codec} altogether * * By default, the implementation returns a {@code newCodec} that replaces * the {@code oldCodec}. * - * @param name - * @param oldCodec - * @param newCodec - * @return + * @param name the codec name + * @param oldCodec the previously registered codec + * @param newCodec the new codec being registered + * @return the codec to register, or {@code null} to remove */server/src/main/java/org/opensearch/index/codec/CodecService.java (1)
112-133: Consider adding debug logging for codec conflicts.The conflict resolution logic is correct. However, when
onConflictis invoked, there's no logging to indicate that a conflict occurred and how it was resolved. This could make debugging challenging in production when multiple plugins contribute codecs.💡 Suggested enhancement
for (CodecRegistry registry : registries) { final Map<String, Codec> additionalCodecs = registry.getCodecs(mapperService, indexSettings, this::defaultCodec); for (Map.Entry<String, Codec> additionalCodec : additionalCodecs.entrySet()) { final String name = additionalCodec.getKey(); final Codec existing = codecs.get(name); if (existing == null) { codecs.put(name, additionalCodec.getValue()); } else { final Codec resolved = registry.onConflict(name, existing, additionalCodec.getValue()); + if (logger != null && logger.isDebugEnabled()) { + logger.debug("Codec conflict for [{}]: resolved to [{}]", name, + resolved == null ? "removed" : (resolved == existing ? "kept existing" : "replaced")); + } if (resolved != null && resolved != existing /* same reference */) {Note: This would require passing a
Loggerparameter through to this section or storing it as a field.server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java (1)
8781-8835: Potential IndexSettings mismatch when constructing CodecService.This builder sets
.indexSettings(indexSettings)but constructsCodecServiceusingconfig.getIndexSettings(). That can diverge if the derived-source test settings affect codec selection/config.Proposed fix
- .codecService(new CodecService(null, config.getIndexSettings(), logger, List.of())) + .codecService(new CodecService(null, indexSettings, logger, List.of()))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
CHANGELOG.mdserver/src/internalClusterTest/java/org/opensearch/indices/IndexingMemoryControllerIT.javaserver/src/main/java/org/opensearch/index/codec/CodecRegistry.javaserver/src/main/java/org/opensearch/index/codec/CodecService.javaserver/src/main/java/org/opensearch/index/codec/CodecServiceConfig.javaserver/src/main/java/org/opensearch/index/engine/EngineConfigFactory.javaserver/src/main/java/org/opensearch/index/shard/IndexShard.javaserver/src/main/java/org/opensearch/plugins/EnginePlugin.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardAllocatorTests.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardBatchAllocatorTests.javaserver/src/test/java/org/opensearch/index/codec/CodecTests.javaserver/src/test/java/org/opensearch/index/engine/CriteriaBasedCompositeIndexWriterBaseTests.javaserver/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.javaserver/src/test/java/org/opensearch/index/engine/InternalEngineTests.javaserver/src/test/java/org/opensearch/index/engine/LeafSorterOptimizationTests.javaserver/src/test/java/org/opensearch/index/engine/NRTReplicationReaderManagerTests.javaserver/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.javaserver/src/test/java/org/opensearch/index/shard/IndexShardTests.javaserver/src/test/java/org/opensearch/index/shard/RefreshListenersTests.javaserver/src/test/java/org/opensearch/index/translog/TranslogOperationHelperTests.javaserver/src/test/java/org/opensearch/indices/IndexingMemoryControllerTests.javaserver/src/test/java/org/opensearch/indices/replication/OngoingSegmentReplicationsTests.javatest/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java
🚧 Files skipped from review as they are similar to previous changes (8)
- server/src/test/java/org/opensearch/index/engine/NRTReplicationReaderManagerTests.java
- server/src/test/java/org/opensearch/indices/replication/OngoingSegmentReplicationsTests.java
- server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java
- server/src/test/java/org/opensearch/index/engine/CriteriaBasedCompositeIndexWriterBaseTests.java
- server/src/test/java/org/opensearch/index/shard/RefreshListenersTests.java
- server/src/test/java/org/opensearch/index/translog/TranslogOperationHelperTests.java
- server/src/test/java/org/opensearch/index/codec/CodecTests.java
- server/src/internalClusterTest/java/org/opensearch/indices/IndexingMemoryControllerIT.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-12-02T22:44:14.799Z
Learnt from: prudhvigodithi
Repo: opensearch-project/OpenSearch PR: 20112
File: server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java:73-81
Timestamp: 2025-12-02T22:44:14.799Z
Learning: In OpenSearch integration tests extending OpenSearchIntegTestCase, using `LuceneTestCase.SuppressCodecs("*")` triggers special handling that selects a random production codec from the CODECS array, while `SuppressCodecs("Asserting")` or other specific codec suppressions still allow Lucene's default codec randomization which may include the asserting codec. Use `SuppressCodecs("*")` when you need to completely avoid asserting codecs (e.g., for cross-thread StoredFieldsReader usage) while maintaining production codec test coverage.
Applied to files:
server/src/main/java/org/opensearch/plugins/EnginePlugin.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardBatchAllocatorTests.javaserver/src/test/java/org/opensearch/index/engine/InternalEngineTests.javaserver/src/main/java/org/opensearch/index/codec/CodecService.javaserver/src/test/java/org/opensearch/index/shard/IndexShardTests.javatest/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardAllocatorTests.javaserver/src/test/java/org/opensearch/indices/IndexingMemoryControllerTests.javaserver/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.javaserver/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: detect-breaking-change
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: Mend Security Check
🔇 Additional comments (26)
server/src/main/java/org/opensearch/index/shard/IndexShard.java (1)
448-448: Factory logic correctly avoids double-composition.The
newCodecServiceOrDefaultmethod properly implements a conditional pattern: ifcodecServiceFactoryis present, it creates a freshCodecServicefrom the factory; otherwise, it returns the passeddefaultCodecServiceunchanged. There is no wrapping or re-composition of the default instance—the two paths are mutually exclusive. ThecodecServicecreated at line 448 serves as a safe fallback when no factory is configured.server/src/test/java/org/opensearch/index/shard/IndexShardTests.java (1)
5139-5164: Remove the recommendation to change the CodecService instantiation in this test.
EngineConfigdoes not expose agetCodecService()method, so the suggested fix is not implementable. Moreover, the testtestCloseShardWhileEngineIsWarmingis specifically verifying engine closure behavior while a warmer is running—not codec registry wiring. Creating a freshCodecService(null, config.getIndexSettings(), logger, List.of())with empty registries is appropriate and safe here;CodecServicereads the registries collection only during construction (it does not store or mutate it), soList.of()is defensible. Plugin registries are not relevant to this unit test's focus.Likely an incorrect or invalid review comment.
server/src/test/java/org/opensearch/gateway/PrimaryShardAllocatorTests.java (1)
815-815: LGTM!The
CodecServiceconstructor calls are correctly updated to include the newList.of()parameter for empty codec registries, aligning with the new four-argument constructor signature.Also applies to: 827-827
server/src/test/java/org/opensearch/index/engine/LeafSorterOptimizationTests.java (1)
31-31: LGTM!The
Listimport and all fourCodecServiceconstructor updates are correct and consistent with the new API signature. Test logic remains unchanged.Also applies to: 62-62, 120-120, 182-182, 259-259
server/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.java (1)
78-78: LGTM!The
CodecServiceconstructor calls are correctly updated with theList.of()parameter. Note that line 229 intentionally usesdefaultIndexSettingsrather thanderivedIndexSettingsfor the CodecService—this appears to be an existing test design choice for the "using source" test case.Also applies to: 186-186, 229-229
server/src/main/java/org/opensearch/index/codec/CodecRegistry.java (1)
33-45: LGTM!The
CodecRegistryinterface design is clean and well-documented. The@ExperimentalApiannotation is appropriate for this new extensibility point, and thegetCodecsmethod signature provides the necessary context (MapperService, IndexSettings, default codec supplier) for plugins to create codec instances.server/src/test/java/org/opensearch/gateway/PrimaryShardBatchAllocatorTests.java (1)
400-400: LGTM!The
CodecServiceconstructor calls are correctly updated with theList.of()parameter, consistent with the changes inPrimaryShardAllocatorTests.java.Also applies to: 412-412
server/src/main/java/org/opensearch/index/codec/CodecService.java (2)
72-78: LGTM on deprecation pattern.The deprecated constructor correctly delegates to the new constructor with an empty registry collection. The
@Deprecated(forRemoval = true)annotation clearly signals migration intent.
80-85: LGTM!The new constructor signature is well-designed, accepting
Collection<CodecRegistry>which provides flexibility for callers while maintaining type safety.server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java (4)
3815-3820: CodecService ctor update looks correct for this test.Passing an empty registries list (
List.of()) aligns with the newCodecServicesignature and keeps existing behavior unchanged.
4314-4331: EngineConfig.Builder: CodecService ctor update looks correct.Empty registries (
List.of()) is appropriate here since the test isn’t exercising plugin-provided codecs.
4349-4374: EngineConfig.Builder: CodecService ctor update looks correct.No behavior change beyond matching the updated constructor signature.
7993-8062: EngineConfig.Builder: CodecService ctor update looks correct.Using
List.of()to represent “no additional codec registries” keeps the test intent intact.server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java (5)
27-27: LGTM! New imports and field for CodecRegistry support.The new
registriesfield and associated imports are correctly added to support the plugin-extensible codec registry feature.Also applies to: 43-43, 61-61
77-77: LGTM! Correct accumulation of CodecRegistry instances from plugins.Unlike
codecServiceandcodecServiceFactorywhich enforce single-plugin override semantics, registries are correctly accumulated from all plugins. Conflict resolution is delegated toCodecRegistry.onConflictduring codec composition, enabling the composability that this PR aims to achieve.Also applies to: 120-122
137-137: LGTM! Properly making the registries list immutable.Wrapping with
Collections.unmodifiableListis the correct approach to prevent external modification of the collected registries.
214-216: LGTM! New public method to create CodecService with registry support.The
newDefaultCodecServicemethod provides a clean API forIndexShardto obtain aCodecServicethat includes all plugin-contributed registries. The method signature is appropriate with@NullableonmapperService.
224-226: LGTM! Registries correctly passed to CodecServiceConfig.The registries are properly propagated to
CodecServiceConfig, ensuring that customCodecServiceFactoryimplementations have access to plugin-contributed registries.test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java (3)
220-220: LGTM! CodecService constructor updated with empty registry list.The
setUp()method correctly passesList.of()to the new 4-argument constructor, aligning with the new API signature.
264-264: LGTM! Allcopy()method overloads correctly updated.The three
copy()method variants consistently use the new 4-argumentCodecServiceconstructor withList.of()for empty registries.Also applies to: 290-290, 316-316
953-953: LGTM! Allconfig()method overloads correctly updated.The
config()methods consistently use the newCodecServiceconstructor signature with empty registries, maintaining uniformity across the test framework.Also applies to: 999-999, 1036-1036
server/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.java (5)
12-13: LGTM! New imports for CodecRegistry testing support.The imports for
Codec,SimpleTextCodec,CodecRegistry,MapperService, and Hamcrest matchers are correctly added to support the new registry-based codec tests.Also applies to: 18-18, 21-21, 37-37, 41-42
129-141: LGTM! Good test coverage for the CodecRegistry pathway.This test validates the end-to-end flow:
BazEnginePlugin.getAdditionalCodecs()→EngineConfigFactorycollects registries →newDefaultCodecService()createsCodecServicewith registries → codec lookup returns the expectedSimpleTextCodec. This effectively tests the core functionality introduced by this PR.
210-212: LGTM! Existing test plugins correctly updated.
FooEnginePlugin,BarEnginePlugin, andBakEnginePluginare updated to use the new 4-argumentCodecServiceconstructor withList.of(), maintaining backward compatibility while adopting the new API.Also applies to: 227-229, 240-247
261-274: LGTM! Clean implementation of the newgetAdditionalCodecsextension point.The
BazEnginePluginproperly demonstrates the newCodecRegistryAPI by returning a registry that provides aSimpleTextCodecunder the key"test". The anonymous class implementation is appropriate for test scaffolding.
117-127: Test behavior is unaffected by the addition ofgetAdditionalCodecs()toBazEnginePlugin.
BazEnginePluginnow provides bothgetCustomTranslogDeletionPolicyFactory()andgetAdditionalCodecs(). The test correctly expects anIllegalStateExceptionwhenFooEnginePluginandBazEnginePluginboth provide conflictingTranslogDeletionPolicyFactoryimplementations. The addition ofgetAdditionalCodecs()does not interfere sinceCodecRegistryinstances are accumulated rather than conflicted, and the test will continue to pass.
3993fe4 to
f5c9e6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
server/src/test/java/org/opensearch/gateway/PrimaryShardBatchAllocatorTests.java (1)
384-415: Test helper CodecService instantiation updated correctly for new ctor.If you want to reduce overhead/noise, consider avoiding
new CodecService(...)just to read a codec name (e.g., centralize a helper for “default codec name” for tests), but this is fine as-is.server/src/main/java/org/opensearch/plugins/EnginePlugin.java (1)
92-102: New hook is a good, non-breaking extension point; tighten Javadoc contract a bit.Consider adding (in the Javadoc) the same kind of “when invoked”/conflict expectations as the other EnginePlugin hooks (e.g., called on index creation, whether multiple plugins may return registries, and how conflicts are handled).
server/src/main/java/org/opensearch/index/codec/CodecService.java (1)
80-136: Registry-based codec composition logic looks correct.The implementation properly handles three cases:
- New codec (no conflict) → add directly
- Conflict resolved to a different codec → replace
- Conflict resolved to
null→ removeOne subtle behavior worth noting: when
resolved == existing(same reference), the codec is kept unchanged. This identity check is intentional to allow registries to explicitly preserve the existing codec, but consider adding a brief inline comment clarifying this semantic for future maintainers.📝 Optional: clarify identity check semantic
if (resolved != null && resolved != existing /* same reference */) { // replace registered codec with the new one codecs.put(name, resolved); - } else if (resolved == null) /* nothing to register */ { + } else if (resolved == null) { + // null signals explicit removal // remove registered codec codecs.remove(name); } + // else: resolved == existing means keep current codec unchangedserver/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java (1)
28-39: Consider adding null-safety for theregistriesfield.The
registriesfield is assigned directly without validation, whileindexSettingsusesObjects.requireNonNull. For consistency and defensive coding, consider either:
- Adding
Objects.requireNonNull(registries)if null is not acceptable, or- Adding
@Nullableannotation if null is validAdditionally, if callers might pass a mutable list, a defensive copy would ensure immutability:
🔒 Suggested defensive handling
- public CodecServiceConfig( - IndexSettings indexSettings, - @Nullable MapperService mapperService, - @Nullable Logger logger, - List<CodecRegistry> registries - ) { - this.indexSettings = Objects.requireNonNull(indexSettings); - this.mapperService = mapperService; - this.logger = logger; - this.registries = registries; - } + public CodecServiceConfig( + IndexSettings indexSettings, + @Nullable MapperService mapperService, + @Nullable Logger logger, + List<CodecRegistry> registries + ) { + this.indexSettings = Objects.requireNonNull(indexSettings); + this.mapperService = mapperService; + this.logger = logger; + this.registries = List.copyOf(Objects.requireNonNull(registries)); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
CHANGELOG.mdserver/src/internalClusterTest/java/org/opensearch/indices/IndexingMemoryControllerIT.javaserver/src/main/java/org/opensearch/index/codec/CodecRegistry.javaserver/src/main/java/org/opensearch/index/codec/CodecService.javaserver/src/main/java/org/opensearch/index/codec/CodecServiceConfig.javaserver/src/main/java/org/opensearch/index/engine/EngineConfigFactory.javaserver/src/main/java/org/opensearch/index/shard/IndexShard.javaserver/src/main/java/org/opensearch/plugins/EnginePlugin.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardAllocatorTests.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardBatchAllocatorTests.javaserver/src/test/java/org/opensearch/index/codec/CodecTests.javaserver/src/test/java/org/opensearch/index/engine/CriteriaBasedCompositeIndexWriterBaseTests.javaserver/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.javaserver/src/test/java/org/opensearch/index/engine/InternalEngineTests.javaserver/src/test/java/org/opensearch/index/engine/LeafSorterOptimizationTests.javaserver/src/test/java/org/opensearch/index/engine/NRTReplicationReaderManagerTests.javaserver/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.javaserver/src/test/java/org/opensearch/index/shard/IndexShardTests.javaserver/src/test/java/org/opensearch/index/shard/RefreshListenersTests.javaserver/src/test/java/org/opensearch/index/translog/TranslogOperationHelperTests.javaserver/src/test/java/org/opensearch/indices/IndexingMemoryControllerTests.javaserver/src/test/java/org/opensearch/indices/replication/OngoingSegmentReplicationsTests.javatest/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java
🚧 Files skipped from review as they are similar to previous changes (8)
- server/src/main/java/org/opensearch/index/codec/CodecRegistry.java
- server/src/main/java/org/opensearch/index/shard/IndexShard.java
- server/src/test/java/org/opensearch/gateway/PrimaryShardAllocatorTests.java
- server/src/test/java/org/opensearch/index/engine/CriteriaBasedCompositeIndexWriterBaseTests.java
- server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java
- server/src/internalClusterTest/java/org/opensearch/indices/IndexingMemoryControllerIT.java
- server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java
- server/src/test/java/org/opensearch/index/translog/TranslogOperationHelperTests.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-02T22:44:14.799Z
Learnt from: prudhvigodithi
Repo: opensearch-project/OpenSearch PR: 20112
File: server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java:73-81
Timestamp: 2025-12-02T22:44:14.799Z
Learning: In OpenSearch integration tests extending OpenSearchIntegTestCase, using `LuceneTestCase.SuppressCodecs("*")` triggers special handling that selects a random production codec from the CODECS array, while `SuppressCodecs("Asserting")` or other specific codec suppressions still allow Lucene's default codec randomization which may include the asserting codec. Use `SuppressCodecs("*")` when you need to completely avoid asserting codecs (e.g., for cross-thread StoredFieldsReader usage) while maintaining production codec test coverage.
Applied to files:
server/src/test/java/org/opensearch/index/shard/IndexShardTests.javaserver/src/main/java/org/opensearch/plugins/EnginePlugin.javaserver/src/test/java/org/opensearch/index/codec/CodecTests.javaserver/src/test/java/org/opensearch/indices/replication/OngoingSegmentReplicationsTests.javatest/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.javaserver/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.javaserver/src/test/java/org/opensearch/indices/IndexingMemoryControllerTests.javaserver/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.javaserver/src/test/java/org/opensearch/index/shard/RefreshListenersTests.javaserver/src/main/java/org/opensearch/index/codec/CodecService.javaserver/src/test/java/org/opensearch/gateway/PrimaryShardBatchAllocatorTests.java
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
Applied to files:
CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: Mend Security Check
🔇 Additional comments (17)
server/src/test/java/org/opensearch/indices/replication/OngoingSegmentReplicationsTests.java (2)
35-35: LGTM!Import correctly added for
List.of()usage and properly sorted among otherjava.utilimports.
77-78: LGTM!Constructor call correctly updated to match the new 4-argument signature. Using
List.of()for the codec registries parameter is appropriate since this test only needs the default codec name and doesn't require any additional codec registrations.CHANGELOG.md (1)
15-15: Changelog entry matches the feature and is correctly categorized under “Added”.server/src/test/java/org/opensearch/indices/IndexingMemoryControllerTests.java (1)
399-426: CodecService construction updated correctly for the new registries parameter.server/src/test/java/org/opensearch/index/shard/IndexShardTests.java (1)
5139-5163: CodecService construction updated correctly for the new registries parameter.server/src/test/java/org/opensearch/index/engine/NRTReplicationReaderManagerTests.java (1)
27-27: LGTM!The import addition and CodecService constructor update correctly align with the new four-argument constructor signature. Using
List.of()provides an immutable empty list, which is appropriate for tests that don't require additional codec registries.Also applies to: 48-48
server/src/test/java/org/opensearch/index/engine/TranslogLeafReaderTests.java (1)
46-46: LGTM!All CodecService constructor calls are consistently updated to use the new four-argument signature with
List.of()for the registries parameter. The changes are mechanical and correctly applied across all test methods.Also applies to: 79-79, 187-187, 230-230
server/src/test/java/org/opensearch/index/engine/LeafSorterOptimizationTests.java (1)
31-31: LGTM!All CodecService constructor invocations are consistently updated to include
List.of()as the registries parameter. The changes maintain test functionality while adapting to the new constructor signature.Also applies to: 62-62, 120-120, 182-182, 259-259
server/src/test/java/org/opensearch/index/shard/RefreshListenersTests.java (1)
162-162: LGTM!The CodecService constructor call is correctly updated to use the new four-argument signature with
List.of()for the registries parameter.server/src/test/java/org/opensearch/index/codec/CodecTests.java (1)
62-62: LGTM!All CodecService constructor calls are consistently updated to include
List.of()as the registries parameter. The test coverage for codec functionality remains intact with proper adaptation to the new API.Also applies to: 174-174, 192-197, 221-221, 229-229
server/src/main/java/org/opensearch/index/codec/CodecService.java (1)
45-46: LGTM!The deprecation annotation with
forRemoval = trueclearly signals the migration path. The deprecated constructor correctly delegates to the new four-argument constructor with an empty list.Also applies to: 72-78
server/src/main/java/org/opensearch/index/codec/CodecServiceConfig.java (1)
55-58: LGTM!The getter provides access to the registries field as expected.
test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java (1)
220-220: LGTM! Consistent updates to the CodecService constructor calls.All seven call sites correctly add
List.of()as the fourth parameter to match the new constructor signature acceptingCollection<CodecRegistry> registries. The empty list preserves existing test behavior while aligning with the new API.Also applies to: 264-264, 290-290, 316-316, 953-953, 999-999, 1036-1036
server/src/test/java/org/opensearch/index/engine/EngineConfigFactoryTests.java (4)
12-13: LGTM! Appropriate imports for the new test functionality.The imports support the new
testCreateEngineConfigFromFactoryAdditionalCodecstest and the CodecRegistry implementation in BazEnginePlugin.Also applies to: 18-18, 21-21, 37-37, 41-42
129-141: LGTM! Good test coverage for the newgetAdditionalCodecshook.The test properly validates that codecs registered via
EnginePlugin.getAdditionalCodecs()are available through theCodecService. The use ofSimpleTextCodecis appropriate for testing as it's a simple, standalone codec that doesn't require additional dependencies.
210-212: LGTM! Consistent constructor updates.The CodecService constructor calls are correctly updated to include
List.of()as the fourth parameter across all plugin implementations.Also applies to: 227-229, 240-247
262-274: LGTM! Clean CodecRegistry implementation for testing.The
getAdditionalCodecsimplementation correctly returns a CodecRegistry that provides a "test" codec mapped toSimpleTextCodec. The implementation only overridesgetCodecs(), which is sufficient since the defaultonConflict()behavior should be appropriate for non-conflicting test scenarios.
|
@msfroh wondering if we could move forward with this one (taking into account rough edges removed and we have an RFC to work on, this change should unblock |
… to allow additional Codec registration (opensearch-project#20411) * Introduce CodecRegistry and EnginePlugin::getAdditionalCodecs hook to allow additional Codec registration Signed-off-by: Andriy Redko <drreta@gmail.com> * Address code review comments Signed-off-by: Andriy Redko <drreta@gmail.com> * Rename CodecRegistry to /AdditionalCodecs, removed the ability to override codecs on conflict (to eliminate any ordering ambiguity) Signed-off-by: Andriy Redko <drreta@gmail.com> * Minor changes to address renaming leftovers Signed-off-by: Andriy Redko <drreta@gmail.com> --------- Signed-off-by: Andriy Redko <drreta@gmail.com>
… to allow additional Codec registration (opensearch-project#20411) * Introduce CodecRegistry and EnginePlugin::getAdditionalCodecs hook to allow additional Codec registration Signed-off-by: Andriy Redko <drreta@gmail.com> * Address code review comments Signed-off-by: Andriy Redko <drreta@gmail.com> * Rename CodecRegistry to /AdditionalCodecs, removed the ability to override codecs on conflict (to eliminate any ordering ambiguity) Signed-off-by: Andriy Redko <drreta@gmail.com> * Minor changes to address renaming leftovers Signed-off-by: Andriy Redko <drreta@gmail.com> --------- Signed-off-by: Andriy Redko <drreta@gmail.com>
Description
Introduce
AdditionalCodecsandEnginePlugin::getAdditionalCodecshook to allow additionalCodecregistration. A while ago we introducedCodecServiceFactoryto provide an ability for the plugins to supply own custom codecs. It does the job and is actively used by:custom-codecsk-nnneural-searchsecurity-analyticsjvectorOne of the major limitations that
CodecServiceFactoryhas is composability - for particular index, only oneCodecServiceFactorycould be provided, otherwise the exception is going to be raise (see please https://forum.opensearch.org/t/knn-doesnt-coexist-with-seismic-sparse/27709 as one of the recent examples).Currently, there are two major uses of
CodecServiceFactory, mutually exclusive in the scope of the same index:custom-codecsfe)k-nn,neural-search,security-analytics,jvector)This pull request is the first step towards replacing
CodecServiceFactorywith composable alternatives: introduce newAdditionalCodecsandEnginePlugin::getAdditionalCodecshook to allow additionalCodecregistration, without the need to provideCodecServiceFactory/ ownCodecServiceinstance.The change is non-breaking and is fully compatible with all existing plugins (although it deprecates the
CodecServiceconstructor). The next steps are:CodecServiceconstructor usage (k-nn,neural-search,security-analytics,jvector)getCustomCodecServiceFactorywithgetAdditionalCodecs(custom-codecs)Related Issues
Part of #20050
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
New Features
Refactor
Public API
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.