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

[martin-gaievski]

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:

1. Get main branch from k-NN repo
2. 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 #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

[reta]

Thanks @martin-gaievski , this is very interesting - the ZSTD (basically, additional codecs) is the sandbox plugin and should not be bundled/installed by default.

[reta]

Ah ... I found it https://github.com/opensearch-project/OpenSearch/blob/main/distribution/build.gradle#L242

if (VersionProperties.isOpenSearchSnapshot()) {
 ...
}

[martin-gaievski]

Thanks @martin-gaievski , this is very interesting - the ZSTD (basically, additional codecs) is the sandbox plugin and should not be bundled/installed by default.

this is a module, not a plugin.

Ah ... I found it https://github.com/opensearch-project/OpenSearch/blob/main/distribution/build.gradle#L242

if (VersionProperties.isOpenSearchSnapshot()) {
 ...
}

right, for module it's different.

[reta]

@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.

[mulugetam]

@reta @martin-gaievski Will create a new PR that does just that.

[reta]

@mulugetam already did

[navneet1v]

@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?

[reta]

Thanks @navneet1v

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

Yes, it is actually the reverse: codec was suggested to Lucene first and rejected due to JNI concerns, see please apache/lucene#439

[navneet1v]

Thanks @navneet1v

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

Yes, it is actually the reverse: codec was suggested to Lucene first and rejected due to JNI concerns, see please apache/lucene#439

Thanks @reta . I was not aware of this.

[kotwanikunal]

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

[dblock]

Do I understand correctly that to enable ZSTD as is I would have to disable k-nn? Do we have an issue open for the long term fix?

[navneet1v]

Do I understand correctly that to enable ZSTD as is I would have to disable k-nn? Do we have an issue open for the long term fix?

There is no issue opened, but we need that. Atleast we got to know about this feature today morning, hence we have not thought about it.

Also, another thing is this is a custom codec, I believe that there should be an index setting which tells this codec should be enabled or not for an index, rather than applying to all indices. Atleast to my knowledge I don't see that in this code change.

@dblock I hope this ans your question.

[mulugetam]

@dblock Yes, I think we should have an issue open for that. @navneet1v User has to set it explicitly: index.codec: ZSTD.

[dblock]

@mulugetam will you please create one for supporting multiple?
@navneet1v will you please create one for the index setting?

[navneet1v]

Hi @mulugetam
As per quick look in the code I cannot see the setting index.codec:ZTSD. Because that has been the case the k-NN build shouldn't not fail. But I can see same setting on K-NN plugin where K-NN tries to give the customer codec factory only if the index.knn setting is true(code).

[navneet1v]

@dblock issue created for supporting zstd codec for K-NN plugin: #7032

I created a generic issue which can allow other plugins who uses the CodecServiceFactory to take advantage of improvement done in Codec of OpenSearch.

[mulugetam]

@dblock @navneet1v #7037.