[Refactor] *Plugin classes in modules/ source module to *Module classes

[nknize]

Is your feature request related to a problem? Please describe.
OpenSearch modules only differ from plugins in that they are installed by default. Aside from that the same plugin scaffolding is used between the two. This makes navigating the source tree difficult. For example, if I'm looking at IngestUserAgentPlugin and IngestAttachmentPlugin I'd have no way of knowing IngestUserAgentPlugin is a module and IngestAttachment is a plugin. (missing javadocs aside).

Describe the solution you'd like
I propose we rote refactor all derived *Plugin classes currently in the modules/ directory to *Module to make it more clear which concrete plugin classes are installed by default vs which are not.

Describe alternatives you've considered
Implement separate module and plugin base classes. I think this is overkill for something that works just fine today and will likely be reworked during the extensions effort.

Additional context
This is a good low hanging fruit effort that a community contributor could pick up for earning merit.

[andrross]

Just thinking out loud, but what about getting rid of the different source tree all together by moving modules under the plugin directory? The "installed by default" property feels like an external configuration and not something fundamental about the module itself. In this proposal we'd have all plugins under the same source tree and then a configuration defined somewhere in a build file (presumably?) that chooses which plugins get installed by default. If we ever change which plugins are installed by default it becomes a simple configuration change with no moving around or renaming of source files.

[nknize]

The "installed by default" property feels like an external configuration and not something fundamental about the module itself.

I don't believe it's an external configuration. I'll have to double check but I believe modules under that directory are auto-installed by the PluginService at bootstrap but plugins under the plugins directory are not (users have to use the InstallPlugin CLI). There is a runtime dependency that we would be breaking if we eliminated the directory. I'm hoping to avoid all that with this simple low hanging fruit issue and just refactor the class names to be more explicit between the auto installed modules vs plugins.

[rockybean]

Seems like an easy task and I'd like to give this a try.

[reta]

@nknize to be fair, this change would make the codebase navigation obscure, at least from developer's standpoint. When I am looking for plugin classes, I just do *Plugin and that's about it (may be it's just me).

Another, more serious problem is confusion with CDI, notably org.opensearch.common.inject.Module (and we have tons of them in the core). Those are indeed modules, not plugins, with this change we would make such distinction quite messy.

To sum up, I am strongly -1 on this idea (sorry about that).

[nknize]

To sum up, I am strongly -1 on this idea (sorry about that).

These are reasonable concerns so no apology necessary.

this change would make the codebase navigation obscure...When I am looking for plugin classes, I just do *Plugin and that's about it

I think this is the problem I'm finding. Using the *Plugin nomenclature provides no developer distinction between those that are modules (installed by default) vs those that are Plugins; and this is causing confusion.

Another, more serious problem is confusion with CDI, notably org.opensearch.common.inject.Module

I agree that the term module is so overloaded here since java modules and the jigsaw framework uses the same vernacular. I think there's a longer term solution to this with extensibility other than the simple rote refactor proposed here. In the meantime I'm looking for something that will solve the third-party confusion of plugins vs core modules beyond the answer "they logically live in the core repository and are shipped with the min distribution".

Would a compromise be to refactor to *ModulePlugin or *PluginModule; e.g., IngestUserAgentPluginModule vs IngestAttachmentPlugin? (feels like a mouth of marbles).

[reta]

Would a compromise be to refactor to *ModulePlugin or *PluginModule; e.g., IngestUserAgentPluginModule vs IngestAttachmentPlugin? (feels like a mouth of marbles).

I think *ModulePlugin (or *BundledPlugin / *InternalPlugin / ...) would be a reasonable compromise, thank you.

[nknize]

Seems like an easy task and I'd like to give this a try.

Thank you @rockybean! It's wonderful to have your contribution. This was also the point of these low hanging fruit issues, for new contributors to participate, iterate, and further learn the code base. I'm curious as to your thoughts as well. Creative ways to make a distinction between OpenSearch Modules vs Plugins w/o adding more confusion regarding class injection (a clumsy implementation on it's own).

I certainly don't want to spend much time bikeshedding but I'm constantly running into this confusion about what should be an external plugin, vs a core plugin, vs a module. The Geo module is getting a lot of refactoring and PR reviews for new contributors become difficult when they see *Plugin because it implies it's an installable plugin.

I think *ModulePlugin (or *BundledPlugin / *InternalPlugin / ...) would be a reasonable compromise, thank you.

*ModulePlugin seems reasonable and an easy refactor. Curious as to other thoughts as well (e.g., do we consider refactoring the CDI module to something else to deconflict w/ OpenSearch Modules)? /cc @andrross

[andrross]

My biggest concern is that from a development/functionality perspective a Module is a Plugin (it's just installed by default), so I did not like the idea of a s/Plugin/Module rename. Renaming to *ModulePlugin alleviates those concerns so I'm on board (and it would be great to have @rockybean's contribution here!)

[andrross]

do we consider refactoring the CDI module to something else to deconflict w/ OpenSearch Modules

I think the root of the confusion here is that "plugin" is a bit overloaded. A org.opensearch.plugins.Plugin is an extension of functionality for OpenSearch that uses the Plugin Java API. Then in the source tree a "plugin" is an optionally-installable org.opensearch.plugins.Plugin whereas a "module" is a default-installed org.opensearch.plugins.Plugin. I don't know how ingrained the definition of "OpenSearch Modules" is for the community at large, but my inclination would be to rename the "module" concept to something like "default plugin" or "internal plugin". Just my two cents (and again I do support doing the *ModulePlugin refactoring now)

[navneet1v]

I certainly don't want to spend much time bikeshedding but I'm constantly running into this confusion about what should be an external plugin, vs a core plugin, vs a module.

+1 on this. This has been a hot topic in the GeoSpatial maintainers. For a new person it becomes very difficult to understand the difference between modules folder and plugin folder. We can change the name to *ModulePlugin or *PluginModule but it will not solve the problem. The confusion will keep on happening, but it would be little less. Apart from just refactoring/not refactoring names we should definitely update this section: https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md#project-layout

+0.5 on the name change.

[saratvemulapalli]

I did open up an issue tangential to the idea here[1].
Do we really need the scaffolding or can we just add a parameter to load them by default?

[1] #1486

[rockybean]

@nknize
Thanks, with the modern IDE support, this is really a low hanging fruit issue. 😃
I also met these kind of confusions when I try to build OpenSearch by myself.
I used to think things under plugins are also installed by default until I try to call snapshot via s3.

+1 for ModulePlugin renaming and I can continue previous PR if all is good for this.

[andrross]

Sure thing @rockybean please feel free to do the renaming. Thanks!

[rockybean]

Has finished the renaming task and please help to check if there is any problem. 🤝 ❤️

[nknize]

implemented in #4042