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

[FEATURE] Create SearchExtension interface #331

Closed
minalsha opened this issue Jan 11, 2023 · 7 comments · Fixed by #387
Closed

[FEATURE] Create SearchExtension interface #331

minalsha opened this issue Jan 11, 2023 · 7 comments · Fixed by #387
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@minalsha
Copy link
Collaborator

Is your feature request related to a problem?

Extension points (see #315) will be (optionally) implemented on Extensions. Eventually these points may need to be communicated to OpenSearch to be registered in the appropriate service or module.

Allowing extensions to inherit from appropriate interfaces depending on their type will match the current Plugin implementation.

What solution would you like?

Add a new interface SearchExtension with default implementations for extension points matching SearchPlugin.java.

@varuntumbe
Copy link
Contributor

Hi @minalsha, Can you assign this issue to me ?

Thanks and regards
Varun

@varuntumbe
Copy link
Contributor

varuntumbe commented Jan 28, 2023

Hi @dbwiddis,

I read through the design concepts and understood most of the things. I have couple of questions to clear.

  1. What exactly are extension points ? So far what I have observed whatever has been defined in Extension Interface are mentioned as extension points and some of them don't have to be registered with open search main node and some of them are. So are extension points are just some properties of extension ?

  2. We have one plugin interface as well many other different types of plugin Interfaces as well, so each interfaces have extension points required for that kind of plugins to work right ? ( like EnginePlugin has getEngineFactory which is needed for providing engine factory for a given index, which is not needed for other Plugins )

  3. So currently we are migrating from plugins to extension, as a part of this we need to create Extension Interface corresponding to Plugin Interface and other equivalent Extension interface corresponding to Plugin Interfaces. So we just need to have the same implementation as Extension here than Plugins right ?

  4. There are no testcases written for the interfaces. So we are not writing for any extension interfaces as well right ? developer who implement it will write testcase for it

Thanks and regards
Varun

@dbwiddis
Copy link
Member

Hey @varuntumbe great questions.

  1. Extension points are methods on plugin interfaces that allow plugins to implement custom behavior. Most of these are optional, as there are default (usually empty list) return values for them. A (nearly?) complete list of those points is in [META] Implement Extension Points #315.
    • Note that we are changing the model from plugins (which run in the same JVM) to extensions (which can run on a different machine entirely) but we want to provide existing plugins the same or similar ability.
    • For now we just want to implement the default behavior. There's a long ongoing effort (see [META] Implement Extension Points #315) to convert all of them over time.
  2. That's correct. For example, plugins that need to handle REST requests implement the ActionPlugin interface. During startup, the getRestHandlers() method on that interface is queried for all plugins that implement ActcionPlugin and registers those handlers with the RestController. (See ActionModule lines 901-913.)
  3. That's correct. These tasks are almost a copy-paste exercise from the plugin interface to extension interface (one reason they are good first issues!)
  4. There are currently no test classes for the plugin implementations, and it seems reasonable not to have them; or maybe just have a single test class that tests all the extension interfaces (and verifies the empty list returns.)

@varuntumbe
Copy link
Contributor

Thanks @dbwiddis for the detailed answers.

I will open a PR for this tonight.

Thanks and regards
Varun

@varuntumbe
Copy link
Contributor

"During startup, the getRestHandlers() method on that interface is queried for all plugins that implement ActcionPlugin and registers those handlers with the RestController"

To handle the above scenario we need to implement #319 to so that open search will have list of extensions and their implemented interfaces correct ?

Thanks and regards
Varun

@dbwiddis
Copy link
Member

"During startup, the getRestHandlers() method on that interface is queried for all plugins that implement ActcionPlugin and registers those handlers with the RestController"

To handle the above scenario we need to implement #319 to so that open search will have list of extensions and their implemented interfaces correct ?

Not quite: in this particular case we have a dedicated transport action that does the registration on initialization, completely bypassing the ActionPlugin iteration. For most "initialization" events we don't need to know the implementation.

However there may be future cases where we use the logic of #319, when OpenSearch needs to communicate something to all initialized extensions, in which case it will be useful to filter the list and avoid unnecessary network traffic.

@varuntumbe
Copy link
Contributor

Got it, got it. Thanks @dbwiddis

Thanks and regards
Varun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants