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

[Draft] Implement getCustomCodecService for EnginePlugin #229

Closed

Conversation

jmazanec15
Copy link
Member

Signed-off-by: John Mazanec [email protected]

Description

This PR implements the getCustomCodecService function of the EnginePlugin interface. That way, we do not have to override the entire engine factory for indices that use k-NN, just the CodecService. This will allow k-NN to be used with CCR plugin.

Issues Resolved

#147

Check List

  • Commits are signed as per the DCO using --signoff

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.

@jmazanec15 jmazanec15 added the Bug Fixes Changes to a system or product designed to handle a programming bug/glitch label Nov 23, 2021
Copy link
Member

@vamshin vamshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks

@codecov-commenter
Copy link

Codecov Report

Merging #229 (ee8ec80) into main (d89f241) will decrease coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #229      +/-   ##
============================================
- Coverage     83.21%   83.00%   -0.21%     
+ Complexity      864      859       -5     
============================================
  Files           123      122       -1     
  Lines          3777     3766      -11     
  Branches        358      358              
============================================
- Hits           3143     3126      -17     
- Misses          474      479       +5     
- Partials        160      161       +1     
Impacted Files Coverage Δ
...main/java/org/opensearch/knn/plugin/KNNPlugin.java 100.00% <100.00%> (ø)
...ava/org/opensearch/knn/plugin/KNNCodecService.java 50.00% <0.00%> (-25.00%) ⬇️
...nsearch/knn/index/codec/KNN87Codec/KNN87Codec.java 80.00% <0.00%> (-16.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d89f241...ee8ec80. Read the comment docs.


@Override
public Engine newReadWriteEngine(EngineConfig config) {
codecService.setPostingsFormat(config.getCodec().postingsFormat());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One call out: The only concern I have with this change is the removal of this line. I believe it will be irrelevant once #146 is fixed. @vamshin do you remember the reason for adding this here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. We would need this line otherwise it could be a breaking change. Reason for explicitly setting this is documented in PR opendistro-for-elasticsearch/k-NN#236

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving PR into draft state for now. I think we can refactor this a little bit.

@jmazanec15 jmazanec15 changed the title Implement getCustomCodecService for EnginePlugin [Draft] Implement getCustomCodecService for EnginePlugin Nov 23, 2021
@jmazanec15
Copy link
Member Author

Closing. I will raise a PR that addresses #146 and #147.

@jmazanec15 jmazanec15 closed this Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fixes Changes to a system or product designed to handle a programming bug/glitch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants