Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] MapperService has to be passed in as null for EnginePlugins CodecService constructor #1805

Closed
jmazanec15 opened this issue Dec 23, 2021 · 3 comments · Fixed by #2177
Closed
Assignees
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request

Comments

@jmazanec15
Copy link
Member

jmazanec15 commented Dec 23, 2021

Describe the bug
Because only index settings are passed into the getCustomCodecService of an EnginePlugin, a non-null MapperService cannot be passed in for the call to super in the custom CodecService constructor.

An issue arises for custom CodecService's that implement their Codec using the delegate pattern. In my case this is the k-NN plugin. The k-NN Codec is really only interested in implementing docValuesFormat and compoundFormat. For the others, it delegates. Because the mapper service is always null, the PerFieldMappingPostingFormatCodec can never be used as the delegate. Because a CodecService is used for the whole index, an index with index.knn set to true cannot get some features provided by PerFieldMappingPostingFormatCodec, even if they are not related to the knn field. We got an issue for this some time back: opendistro-for-elasticsearch/k-NN#227. Our solution was to set the PostingFormat from the custom EngineFactory. However, as I am trying to remove the custom engine factory and just rely on custom Codec Service, this solution will no longer work.

To Reproduce
See opendistro-for-elasticsearch/k-NN#227 as an example.

Expected behavior
An EnginePlugin's custom CodecService can be constructed with a non-null mapperService.

I think this could be fixed by changing where the CodecService's are constructed or perhaps using some kind of CodecService Supplier in the EngineConfigFactory. However, I cannot think of a clean way of fixing it without changing the EnginePlugin interface.

Plugins
N/A

Screenshots
N/A

Host/Environment (please complete the following information):
N/A

@jmazanec15 jmazanec15 added bug Something isn't working untriaged labels Dec 23, 2021
@reta
Copy link
Collaborator

reta commented Dec 28, 2021

@jmazanec15 from the implementation perspective, the IndexService.needsMapperService is a decision point if the MapperService is needed / not needed in the specific context. Fe, the closed indices do not need the MapperService for the reason that closed indices are immutable.

But the EngineConfigFactory could provide the instance of the MapperService (if available, using IndicesService internally) to the EnginePlugin, if it makes sense, @nknize @andrross @dblock what do you think guys?

@meghasaik
Copy link
Contributor

As mentioned above, we can use the MapperService from the IndicesService and provide it to the EngineConfigFactory.

Correct me if I am wrong but one approach would be that the MapperService through EngineConfigFactory can be provided to the CodeService through a CodecSupplier directly depending on the MapperService value assuming it is non-null. The other approach is described as above but the EnginePlugin interface might have to be changed in order to incorporate the MapperService as the EnginePlugin only takes the index-settings for now.

@anasalkouz anasalkouz added enhancement Enhancement or improvement to existing feature or request and removed bug Something isn't working labels Jan 10, 2022
@reta
Copy link
Collaborator

reta commented Feb 16, 2022

Experimenting with adding CodecServiceFactory into EnginePlugin (non-breaking change) .... @meghasaik are you working on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants