Skip to content

Make NamedXContent available to extensions #244

Merged
joshpalis merged 4 commits intoopensearch-project:mainfrom
dbwiddis:namedXContent
Nov 18, 2022
Merged

Make NamedXContent available to extensions #244
joshpalis merged 4 commits intoopensearch-project:mainfrom
dbwiddis:namedXContent

Conversation

@dbwiddis
Copy link
Copy Markdown
Member

@dbwiddis dbwiddis commented Nov 12, 2022

Description

Implements the getNamedXContent() extension point:

  1. Adds getNamedXContent() to the Extension interface, so that the ExtensionsRunner can read it. This will allow direct use of existing plugin method of this name.
  2. Creates an ExtensnionNamedXContentRegistry which combines the Extension's custom content and the core XContent from OpenSearch. Getters fetch either the combined registry (needed as an input for various parsers) or just the custom content (presently unused, but potentially available for transport request/response (no need, functionality already on ExtensionsRunner)
  3. Exposes the ExtensionNamedXContentRegistry via a getter to extensions so they can easily integrate them into REST Handlers by:
    • Passing the extensionsRunner object from the Extension (inherited from BaseExtension) to the RestHandler
    • Using extensionsRunner.getNamedXContentRegistry().getRegistry() to return the (combined) registry to pass to parsers

See AD #725 for an implementation leveraging these improvements!

Issues Resolved

Fixes #208
Fixes #131
Fixes #94

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.

Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis dbwiddis requested a review from a team November 12, 2022 04:38
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 12, 2022

Codecov Report

Merging #244 (cbf203f) into main (6df1fa4) will increase coverage by 3.65%.
The diff coverage is 100.00%.

❗ Current head cbf203f differs from pull request most recent head 1a83f71. Consider uploading reports for the commit 1a83f71 to get more accurate results

@@             Coverage Diff              @@
##               main     #244      +/-   ##
============================================
+ Coverage     67.19%   70.84%   +3.65%     
- Complexity      102      115      +13     
============================================
  Files            24       25       +1     
  Lines           506      542      +36     
  Branches         17       17              
============================================
+ Hits            340      384      +44     
+ Misses          154      146       -8     
  Partials         12       12              
Impacted Files Coverage Δ
...rc/main/java/org/opensearch/sdk/BaseExtension.java 77.77% <100.00%> (+2.77%) ⬆️
src/main/java/org/opensearch/sdk/Extension.java 63.63% <100.00%> (+3.63%) ⬆️
...opensearch/sdk/ExtensionNamedXContentRegistry.java 100.00% <100.00%> (ø)
...main/java/org/opensearch/sdk/ExtensionsRunner.java 71.05% <100.00%> (+5.86%) ⬆️
...rch/sdk/handlers/ExtensionsInitRequestHandler.java 100.00% <100.00%> (ø)
...k/handlers/EnvironmentSettingsResponseHandler.java 64.70% <0.00%> (+29.41%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@owaiskazi19
Copy link
Copy Markdown
Member

@dbwiddis if we can add a design for getNamedXContent() in DESIGN.md would be great.

@owaiskazi19
Copy link
Copy Markdown
Member

#94 talks about retry for handling transport errors by doing a retry. Are we handling it in this PR?

@dbwiddis
Copy link
Copy Markdown
Member Author

@dbwiddis if we can add a design for getNamedXContent() in DESIGN.md would be great.

Sure, I'll provide one. It's a good template for other similar extension points.

https://i.imgflip.com/70rp2r.jpg

@dbwiddis
Copy link
Copy Markdown
Member Author

#94 talks about retry for handling transport errors by doing a retry. Are we handling it in this PR?

No, but #174 addresses this.

owaiskazi19
owaiskazi19 previously approved these changes Nov 15, 2022
ryanbogan
ryanbogan previously approved these changes Nov 16, 2022
owaiskazi19
owaiskazi19 previously approved these changes Nov 17, 2022
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@joshpalis joshpalis merged commit 4810585 into opensearch-project:main Nov 18, 2022
@dbwiddis dbwiddis deleted the namedXContent branch February 19, 2023 22:42
kokibas pushed a commit to kokibas/opensearch-sdk-java that referenced this pull request Mar 17, 2023
* Make NamedXContent available to extensions

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add tests

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* No need to save custom list in registry as it's on ExtensionsRunner

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add sequence diagram to DESIGN.md

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Signed-off-by: Daniel Widdis <widdis@gmail.com>
caokyhieu pushed a commit to caokyhieu/opensearch-sdk-java that referenced this pull request Aug 15, 2025
* Make NamedXContent available to extensions

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add tests

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* No need to save custom list in registry as it's on ExtensionsRunner

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add sequence diagram to DESIGN.md

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants