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

[Extensions ] Adds HttpPort setting to ExtensionInitializationRequest #7770

Closed
wants to merge 1 commit into from

Conversation

joshpalis
Copy link
Member

@joshpalis joshpalis commented May 26, 2023

Description

Companion SDK PR : opensearch-project/opensearch-sdk-java#788

Retrieves the http.host setting from Node.java and sends this value (present or default) to the extension during initialization for use in updating the extension settings, rest/java clients.

See related comment here

Related Issues

opensearch-project/opensearch-sdk-java#782

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchTaskCancellationWithHighCpu

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Merging #7770 (71c1c37) into main (277eb3d) will increase coverage by 0.00%.
The diff coverage is 41.66%.

@@            Coverage Diff            @@
##               main    #7770   +/-   ##
=========================================
  Coverage     70.87%   70.87%           
- Complexity    56203    56211    +8     
=========================================
  Files          4682     4682           
  Lines        266099   266106    +7     
  Branches      39072    39075    +3     
=========================================
+ Hits         188595   188613   +18     
+ Misses        61520    61471   -49     
- Partials      15984    16022   +38     
Impacted Files Coverage Δ
...a/org/opensearch/extensions/ExtensionsManager.java 56.80% <0.00%> (-0.27%) ⬇️
...ensearch/discovery/InitializeExtensionRequest.java 69.23% <50.00%> (-0.77%) ⬇️

... and 453 files with indirect coverage changes

@ryanbogan ryanbogan changed the title [Feature/Extensions ] Adds HttpPort setting to ExtensionInitializationRequest [Extensions ] Adds HttpPort setting to ExtensionInitializationRequest May 26, 2023
@joshpalis joshpalis closed this May 26, 2023
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections to this as a temporary thing if you need it to unblock your work, however, the first thing the SDK's init request handler does after handling this request, is to request the environment settings.

Can you just delay whatever you're using this String for in the SDK until after you get the settings? Pretty sure for HTTP you don't need it immediately.

It's possible you might want "transport port" earlier, in which case you might want to proactively send the whole environment settings either here with this request, or in an immediate followup right after this init is acked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants