-
Notifications
You must be signed in to change notification settings - Fork 75
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] Improves performance by replacing cluster state calls to retrieve cluster name #924
[Extensions] Improves performance by replacing cluster state calls to retrieve cluster name #924
Conversation
Signed-off-by: Joshua Palis <[email protected]>
Codecov Report
@@ Coverage Diff @@
## feature/extensions #924 +/- ##
========================================================
- Coverage 42.86% 42.84% -0.02%
Complexity 2420 2420
========================================================
Files 301 301
Lines 18169 18176 +7
Branches 1873 1873
========================================================
Hits 7788 7788
- Misses 9806 9813 +7
Partials 575 575
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…ings are populated after initialization Signed-off-by: Joshua Palis <[email protected]>
} | ||
|
||
protected ADCancelTaskResponse newResponse( | ||
ADCancelTaskRequest request, | ||
List<ADCancelTaskNodeResponse> responses, | ||
List<FailedNodeException> failures | ||
) { | ||
return new ADCancelTaskResponse(clusterService.state().getClusterName(), responses, failures); | ||
return new ADCancelTaskResponse( | ||
new ClusterName(extensionsRunner.getEnvironmentSettings().get("cluster.name")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating a new object every time. Can we have it in createComponent and pass along?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've explored this option but createComponents
is invoked during ExtensionsRunner
initialization, therefore we would be unable to retrieve the cluster name from the environment settings until after extension initialization since environment settings are set at that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check if we can store the cluster name as a global variable somewhere, the reason being cluster name would remain same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I think the best place to store this would be in the SDKClusterService
itself. I'll add a cluster name variable to the SDKClusterService
and set this during extension initialization as part of updateSDKClusterService()
, which currently only updates the cluster settings. This way, we can just pull this information from the cluster service and avoid having to pass the extension runner to these transport actions.
…SDKClusterService directly Signed-off-by: Joshua Palis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much cleaner now
Description
SDK Companion PR : opensearch-project/opensearch-sdk-java#818 (SDK PR must be merged first)
Retrieves cluster name from the SDKClusterService rather than from the cluster state
See comment here for more details
Issues Resolved
Part of opensearch-project/opensearch-sdk-java#674
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.