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] Give Extensions access to core NamedXContent #208

Closed
dbwiddis opened this issue Oct 31, 2022 · 2 comments · Fixed by #244
Closed

[FEATURE] Give Extensions access to core NamedXContent #208

dbwiddis opened this issue Oct 31, 2022 · 2 comments · Fixed by #244
Assignees
Labels
enhancement New feature or request

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Oct 31, 2022

Is your feature request related to a problem?

Extensions need access to core NamedXContent, such as predefined range filters, cumulative sums, etc. These will need to be part of the content parser used with the ExtensionRestRequest:

#163 is now merged, so in an extension when processing an ExtensionRestRequest we will have a getXContentType() getter, returning either null, or one of the XContentType enums. The equivalent of contentParser() is:

request.getXContentType().xContent().createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, content.streamInput())

To create the xContentRegistry parameter in the above, note the AD Plugin has a getNamedXContent() method already: https://github.com/opensearch-project/anomaly-detection/blob/08fdbdde4c2f97c27f63d3a4ba31f8acd6dbf570/src/main/java/org/opensearch/ad/AnomalyDetectorPlugin.java#L938-L946

Add that to an empty registry as follows

NamedXContentRegistry xContentRegistry() {
    SearchModule searchModule = new SearchModule(Settings.EMPTY, Collections.emptyList());
    List<NamedXContentRegistry.Entry> entries = searchModule.getNamedXContents();
    entries.addAll(getNamedXContent());
    return new NamedXContentRegistry(entries);
}

Creating the registry like the above will probably end up being a function of createComponents() as part of issue #160 with the ExtensionsRunner class reading the getNamedXContent() from the extension (default empty list). cc: @owaiskazi19 @joshpalis @ryanbogan

Originally posted by @dbwiddis in #58 (comment)

These should be initialized in the BaseExtension or as part of CreateComponents.

What solution would you like?

Generate the registry on the SDK side. It's easy except for the Search Module, but we can instantiate one (such as in the above example) and figure out how to instantiate its standard methods already in core.

In order to generate this Xcontent parser on the SDK side, we would first need to transport the consolidated namedXcontentRegistry ...

This is what I'm saying is ugly.

I'm suggesting we generate the whole registry fresh on the SDK side.

There are a fixed number of registries, see here:
https://github.com/opensearch-project/OpenSearch/tree/main/libs/x-content/src/main/java/org/opensearch/common/xcontent

The registry is initialized in Node.java by iterating known getters:

NamedXContentRegistry xContentRegistry = new NamedXContentRegistry(
    Stream.of(
        NetworkModule.getNamedXContents().stream(),
        IndicesModule.getNamedXContents().stream(),
        searchModule.getNamedXContents().stream(),
        pluginsService.filterPlugins(Plugin.class).stream().flatMap(p -> p.getNamedXContent().stream()),
        ClusterModule.getNamedXWriteables().stream()
    ).flatMap(Function.identity()).collect(toList())
);

The ones starting from uppercase class names (Network, Indices, Cluster) are static and can be called from the SDK side exactly as they are here.

Originally posted by @dbwiddis in #58 (comment)

What alternatives have you considered?

Wasting a lot of bandwidth communicating the entire registry (and content) over transport.

Do you have any additional context?

See opensearch-project/anomaly-detection#692 for some cherry-picked NamedXContent that would not be needed if we just had the whole registry available in a BaseExtension

@dbwiddis dbwiddis added the enhancement New feature or request label Oct 31, 2022
@dbwiddis dbwiddis self-assigned this Oct 31, 2022
@dbwiddis dbwiddis changed the title [FEATURE] [FEATURE] Give Extensions access to core NamedXContent Oct 31, 2022
@dbwiddis
Copy link
Member Author

dbwiddis commented Nov 1, 2022

Extensions currently have an ExtensionNamedWriteableRegistry class that we can leverage.

  • There is an instance field namedWriteables intended to be the extension's named writeables, that is returned to OpenSearch via response handler if requested
  • There is a placeholder getNamedWritables() method that was envisioned to get the extension's named writeables, but is currently an empty list. That will be changed to a simple getter.
  • The no-arg constructor can be removed, leaving only the constructor that populates the namedWriteables method from a list.
    • That list will be fetched from an extension via getNamedWritables on the Extension interface (with a default empty list). This will be an exact copy of plugin implementation.
    • Internally the ExtensionNamedWriteableRegistry will also re-generate all the named writeables from OpenSearch. (We could request them sent over transport, but it's easy to just re-create them) and merge the two copies into a registry that can be sent to the extension via CreateComponents.

@dbwiddis
Copy link
Member Author

Extensions currently have an ExtensionNamedWriteableRegistry class that we can leverage.

Actually, I was confused. NamedWriteables are different than NamedXWriteables (same as NamedXContent) and there's no overlap.

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

Successfully merging a pull request may close this issue.

1 participant