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

Add ClusterStateRequest parameter to cluster state transport request #668

Closed

Conversation

dbwiddis
Copy link
Member

Companion PR on OpenSearch: opensearch-project/OpenSearch#7066

Description

Adds the ability to send a ClusterStateRequest parameter when requesting Cluster State, to limit the values returned (and reduce transport bandwidth). To get the old behavior, use new ClusterStateRequest().all() as the parameter.

Issues Resolved

Fixes #354 (caveat: we should decide whether to add a call to this method in the SDKClient (which requires passing the TransportService around). For now it is accessible to extensions via ExtensionsRunner.getSdkTransportService().

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.

@codecov-commenter
Copy link

Codecov Report

Merging #668 (cf3943a) into main (94618e0) will increase coverage by 2.57%.
The diff coverage is 63.63%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #668      +/-   ##
============================================
+ Coverage     64.17%   66.75%   +2.57%     
- Complexity      274      281       +7     
============================================
  Files            55       55              
  Lines          1164     1164              
  Branches         38       38              
============================================
+ Hits            747      777      +30     
+ Misses          404      374      -30     
  Partials         13       13              
Impacted Files Coverage Δ
...main/java/org/opensearch/sdk/ExtensionsRunner.java 78.64% <ø> (+1.41%) ⬆️
...arch/sdk/handlers/ClusterStateResponseHandler.java 52.94% <ø> (+17.64%) ⬆️
...n/java/org/opensearch/sdk/SDKTransportService.java 78.84% <60.00%> (+31.22%) ⬆️
...ain/java/org/opensearch/sdk/SDKClusterService.java 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

try {
transportService.sendRequest(
opensearchNode,
ExtensionsManager.REQUEST_EXTENSION_CLUSTER_STATE,
Copy link
Member

Choose a reason for hiding this comment

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

This will conflict with #647. We'll probably get your changes in and I'll rebase.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I thought so.

Also, I'll be doing another PR for GetFieldMappings which will follow this template, and we (you) may want to make a more generic protobuf action for all the TransportActions (with associated ActionRequest/ActionResponse) with similar casing to this... they all implement Writeable.

Or we could adapt the ProxyAction code to do this. I started down that road but it got pretty spaghetti pretty fast.

@dbwiddis
Copy link
Member Author

Based on feedback about sending cluster state over transport, I'm closing this PR in favor of targeted API requests for the information we need.

@dbwiddis dbwiddis closed this Apr 13, 2023
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.

[FEATURE] Add ability to pass ClusterStateRequest object when requesting Cluster State
5 participants