OpenSearch: [BUG] Zstd new codec is a breaking change for kNN plugin

Describe the bug Plugins cannot continue to use existing approach and register CodecServiceFactory and cluster is crushing in runtime (compile time is ok). Example is kNN plugin, error on cluster start:

https://github.com/opensearch-project/k-NN/actions/runs/4614564115/jobs/8170344936#step:9:1265

»  java.lang.IllegalStateException: existing codec service factory already overridden in: org.opensearch.index.codec.customcodecs.CustomCodecPlugin attempting to override again by: org.opensearch.knn.plugin.KNNPlugin
»  	at org.opensearch.index.engine.EngineConfigFactory.<init>(EngineConfigFactory.java:107) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.index.engine.EngineConfigFactory.<init>(EngineConfigFactory.java:63) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.indices.IndicesService.getEngineConfigFactory(IndicesService.java:913) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.indices.IndicesService.createIndexService(IndicesService.java:874) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.indices.IndicesService.createIndex(IndicesService.java:775) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.indices.IndicesService.createIndex(IndicesService.java:209) ~[opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
»  	at org.opensearch.indices.cluster.IndicesClusterStateService.createIndices(IndicesClusterStateService.java:539) [opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]

Code on kNN side that is registering CustomServiceFactory: https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/plugin/KNNPlugin.java#L271-L276

To Reproduce Steps to reproduce the behavior:

Get main branch from k-NN repo
Do ./gradlew run

Expected behavior There should be alternative way of either enabling/disabling plugins or for plugin to register their CustomServiceFactory.

Plugins distribution build, k-NN is included

Additional context Core change introduced in https://github.com/opensearch-project/OpenSearch/pull/3577

Part of PR that is registering new factory: https://github.com/opensearch-project/OpenSearch/pull/3577/files#diff-629964cdbeae1039851179519fc3e3d3cc45e8329d208dd9f2b7034537675262R38

Core code where the check for only one factory is happening: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/index/engine/EngineConfigFactory.java#L79

About this issue

Original URL
State: closed
Created a year ago
Comments: 17 (12 by maintainers)

Commits related to this issue

Bug fix: enable ZSTD codec only if index.codec is set to ZSTD or ZSTDNODICT. - addresses https://github.com/opensearch-project/OpenSearch/issues/7012 Signed-off-by: Mulugeta Mammo <mulugeta.mammo@... — committed to mulugetam/OpenSearch by mulugetam a year ago
Fix: enable ZSTD codec only if index.codec is set to ZSTD. - addresses https://github.com/opensearch-project/OpenSearch/issues/7012 Signed-off-by: Mulugeta Mammo <mulugeta.mammo@intel.com> — committed to mulugetam/OpenSearch by mulugetam a year ago
Fix: avoid ZSTD codec from overriding service codec factory. (#7037) * Fix: enable ZSTD codec only if index.codec is set to ZSTD. - addresses https://github.com/opensearch-project/OpenSearch/iss... — committed to opensearch-project/OpenSearch by mulugetam a year ago
Fix: avoid ZSTD codec from overriding service codec factory. (#7037) * Fix: enable ZSTD codec only if index.codec is set to ZSTD. - addresses https://github.com/opensearch-project/OpenSearch/issue... — committed to opensearch-project/OpenSearch by github-actions[bot] a year ago
Fix: avoid ZSTD codec from overriding service codec factory. (#7037) (#7149) * Fix: enable ZSTD codec only if index.codec is set to ZSTD. - addresses https://github.com/opensearch-project/OpenSe... — committed to opensearch-project/OpenSearch by opensearch-trigger-bot[bot] a year ago
Fix: avoid ZSTD codec from overriding service codec factory. (#7037) * Fix: enable ZSTD codec only if index.codec is set to ZSTD. - addresses https://github.com/opensearch-project/OpenSearch/iss... — committed to austintlee/OpenSearch by mulugetam a year ago

Most upvoted comments

@dblock @navneet1v https://github.com/opensearch-project/OpenSearch/pull/7037.

mulugetam on Apr 6, 2023

@martin-gaievski The fix has been merged in and backported. Please reopen if the issue still exists.

kotwanikunal on Apr 5, 2023

@mulugetam I think we should move custom-codecs from sandbox/modules to sandbox/plugins, at least for for now, otherwise the way modules are installed breaks other plugins.

@reta This should work once custom-codecs is moved to Plugins folder in sandbox.

From feature standpoint this looks an interesting feature. Once the change is made we should also think about how other plugins which provide their own codec can take advantage of this new zstd codec. Happy to discuss this more on a separate issue or this same issue.

Another point, did we consider adding this codec as part of Lucene itself?

navneet1v on Apr 5, 2023

@mulugetam already did

reta on Apr 5, 2023

@mulugetam I think we should move custom-codecs from sandbox/modules to sandbox/plugins, at least for for now, otherwise the way modules are installed breaks other plugins.

reta on Apr 5, 2023