Adding ExtensiblePlugin to KNNPlugin

[martin-gaievski]

Signed-off-by: Martin Gaievski gaievski@amazon.com

Description

With this change making KNNPlugin implementing ExtensiblePlugin interface. This enables plugin extension to contributors who had forked from main OS repository. ExtensiblePlugin has default implementation for its only method loadExtensions, and KNNPlugin does not override it with this initial implementation.

Issues Resolved

#180

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.

[codecov-commenter]

Codecov Report

Merging #264 (f6c261c) into main (d3cc827) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main     #264   +/-   ##
=========================================
  Coverage     83.32%   83.32%           
  Complexity      883      883           
=========================================
  Files           127      127           
  Lines          3832     3832           
  Branches        361      361           
=========================================
  Hits           3193     3193           
  Misses          477      477           
  Partials        162      162           

Impacted Files	Coverage Δ
...main/java/org/opensearch/knn/plugin/KNNPlugin.java	100.00% <100.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 d3cc827...f6c261c. Read the comment docs.

[vamshin]

LGTM! Thanks

[jmazanec15]

Before approving, I want to understand what impact implementing a plugin and then using the default for its only method has. Can you explain what effect this has?

[martin-gaievski]

Before approving, I want to understand what impact implementing a plugin and then using the default for its only method has. Can you explain what effect this has?

sure thing, this change is required for the plugin to be extensible by third-party. There are two types of plugin extensions that external developer can do - write officially recommended extension using open-search extension framework (something that we have done in knn ourselves for the painless plugin) or fork our repo and create custom k-NN plugin by changing/adding classes directly to the forked codebase.

Option 2 is the most straightforward and it works. However for option 1 we need to do some modifications on our end. Recently I've suggested one possible workaround that presumably could work without changes on our side. One of the requesters has tested it with their plugin extensions and unfortunately it doesn't work and fails with the same error. This means that in order to make option 1 workable we need to modify our code.

Problem is that in order to have meaningful extensions we need to provide extension points inside our codebase, where potential extensions will be called (something like REST request interceptor). More details are in this document.
As we're not planning to add extension points to the plugin the minimum change that we can do it to make KNNPlugin class implement the ExtensiblePlugin.
The interface has only one method loadExtensions, and typically plugin that implements it initializes instances of extensions via Java SPI (e.g. PainlessPlugin). Interface has the default implementation that does nothing, in such case no extensions will be loaded. At the same time this will allow to pass validation in PluginService class from main OpenSearch repo that is currently a blocker.

[martin-gaievski]

Before approving, I want to understand what impact implementing a plugin and then using the default for its only method has. Can you explain what effect this has?

sure thing, this change is required for the plugin to be extensible by third-party. There are two types of plugin extensions that external developer can do - write officially recommended extension using open-search extension framework (something that we have done in knn ourselves for the painless plugin) or fork our repo and create custom k-NN plugin by changing/adding classes directly to the forked codebase.

Option 2 is the most straightforward and it works. However for option 1 we need to do some modifications on our end. Recently I've suggested one possible workaround that presumably could work without changes on our side. One of the requesters has tested it with their plugin extensions and unfortunately it doesn't work and fails with the same error. This means that in order to make option 1 workable we need to modify our code.

Problem is that in order to have meaningful extensions we need to provide extension points inside our codebase, where potential extensions will be called (something like REST request interceptor). More details are in this document. As we're not planning to add extension points to the plugin the minimum change that we can do it to make KNNPlugin class implement the ExtensiblePlugin. The interface has only one method loadExtensions, and typically plugin that implements it initializes instances of extensions via Java SPI (e.g. PainlessPlugin). Interface has the default implementation that does nothing, in such case no extensions will be loaded. At the same time this will allow to pass validation in PluginService class from main OpenSearch repo that is currently a blocker.

Updated quip doc link to the public github gist of the same content