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

Make NamedXContent available to extensions #244

Merged
merged 4 commits into from
Nov 18, 2022

Conversation

dbwiddis
Copy link
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.

@dbwiddis dbwiddis requested a review from a team November 12, 2022 04:38
@codecov-commenter
Copy link

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 <[email protected]>
@owaiskazi19
Copy link
Member

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

@owaiskazi19
Copy link
Member

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

@dbwiddis
Copy link
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
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
DESIGN.md Outdated Show resolved Hide resolved
ryanbogan
ryanbogan previously approved these changes Nov 16, 2022
owaiskazi19
owaiskazi19 previously approved these changes Nov 17, 2022
@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 <[email protected]>

* Add tests

Signed-off-by: Daniel Widdis <[email protected]>

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

Signed-off-by: Daniel Widdis <[email protected]>

* Add sequence diagram to DESIGN.md

Signed-off-by: Daniel Widdis <[email protected]>

Signed-off-by: Daniel Widdis <[email protected]>
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