Migrate from CodecServiceFactory to AdditionalCodecs (so the custom codecs could be used with k-nn, neutral-search, ... plugins)

[reta]

Description

Include AdditionalCodecs argument to allow additional Codec registration
Migrate from CodecServiceFactory to AdditionalCodecs (so the custom codecs could be used with k-nn, neutral-search, ... plugins)

Related Issues

Followup on opensearch-project/OpenSearch#20411

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

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.

[mgodwan]

Thanks @reta for the change. I just started following the changes in core. For my understanding:

1. How would this handle composition for non per field formats? (e.g. if a codec is overridding SegmentInfosFormat, StoredFieldsFormat). We need some validation I believe at index creation time to ensure that
2. Would this still require a primary CodecService for the defaultCodec being passed in the getAdditionalCodecs method? As otherwise it may just end up working with the default codec. I was looking at KnnCodec and it also does not override the default codec method and hence wanted to know how would the composition work there.

Change LGTM otherwise wrt to the proposal share in the issue in core repository.

[reta]

Thanks @mgodwan !

1. How would this handle composition for non per field formats? (e.g. if a codec is overridding SegmentInfosFormat, StoredFieldsFormat). We need some validation I believe at index creation time to ensure that

To my understanding, when formats are overridden, there two options:

• delegate to default codec (whatever this codec could be)
• replace the implementation altogether (no delegation)

If I understand your question correctly, if the wrapping codec needs to have some in-depth, exact assumptions what the default codec is doing - yes, we need to make sure there is validation in place (the index.codec settings check would be needed). I have not seen examples of that, would appreciate if you have pointers. On the other side, If that is just delegation as-is, we should be totally fine.

2. Would this still require a primary CodecService for the defaultCodec being passed in the getAdditionalCodecs method? As otherwise it may just end up working with the default codec. I was looking at KnnCodec and it also does not override the default codec method and hence wanted to know how would the composition work there.

So the default codec is not passed to getAdditionalCodecs , primarily because CodecService (and default codec) are not available at the time this hook is called. However, the getAdditionalCodecs returns AdditionalCodecs abstraction that will be called by CodecService while constructing the codecs for the index, at this point the correct default codec will be passed over there. Basically, to answer you question in a simple way, yes, the default codec will be available at the moment additional codecs needed to be built.

Hope I answered your questions, thanks for looking!

[reta]

@mgodwan my apologies, I think I have better response to your question / concerns. There should be no issue with codec overriding SegmentInfosFormat, StoredFieldsFormat, however there are issues (unfortunately) when codecs are composed on top of / wrapping each other, like k-nn fe (I have this RFC to explore the problem if you are interested [1]).

The issue with that is on the read side, when Lucene tries to read segments, it explicitly calls Codec.forName and unless the default / known codec is being wrapped, we are having a problem at the moment. It is not specific to custom-codecs plugin, but basically any non-default codec. I am looking what options do we have here.

[1] opensearch-project/OpenSearch#20491

[reta]

@sarthakaggarwal97 @andrross folks could you please help me to get this change in for 3.5? thank you

[sarthakaggarwal97]

LGTM, I just checked the implementation wrt to recent changes in OpenSearch. Just a minor nit around comments. Thanks @reta