-
Notifications
You must be signed in to change notification settings - Fork 73
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
[Feature/extensions] Implements create components for AnomalyDetectionIndices and ADTaskManager #825
[Feature/extensions] Implements create components for AnomalyDetectionIndices and ADTaskManager #825
Conversation
… components until SDKClusterSettings support is added Signed-off-by: Joshua Palis <jpalis@amazon.com>
…iggered from SDKRestClient Signed-off-by: Joshua Palis <jpalis@amazon.com>
src/test/java/org/opensearch/ad/caching/PriorityCacheTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/ad/task/ADTaskCacheManagerTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/ad/caching/PriorityCacheTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/ad/task/ADTaskCacheManagerTests.java
Outdated
Show resolved
Hide resolved
… test class, passed in SDKNamedXContentRegistry to ADTaskManagerreturning SDKRestClient as a created component to enable dependency injection, using multi-line comments s in PriorityCacheTests Signed-off-by: Joshua Palis <jpalis@amazon.com>
Signed-off-by: Joshua Palis <jpalis@amazon.com>
Codecov Report
📣 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 @@
## feature/extensions #825 +/- ##
========================================================
- Coverage 45.60% 39.89% -5.72%
+ Complexity 2458 2110 -348
========================================================
Files 297 297
Lines 17115 17094 -21
Branches 1843 1843
========================================================
- Hits 7805 6819 -986
- Misses 8740 9787 +1047
+ Partials 570 488 -82
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: Joshua Palis <jpalis@amazon.com>
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.
LGTM except for field/param naming. The vast majority of the diff on the ADTaskManager
class can be eliminated by keeping the old names and just changing the class type.
private final Client client; | ||
private final ClusterService clusterService; | ||
private final NamedXContentRegistry xContentRegistry; | ||
private final SDKRestClient sdkRestClient; |
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 we keep the field name client
here to minimize the diff with AD plugin?
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.
Will do
ClusterService clusterService, | ||
Client client, | ||
NamedXContentRegistry xContentRegistry, | ||
Settings environmentSettings, |
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 we keep the parameter name settings
here to minimize the diff with AD plugin?
@@ -1671,7 +1671,9 @@ protected <T> void deleteTaskDocs( | |||
BulkRequest bulkRequest = new BulkRequest(); | |||
while (iterator.hasNext()) { | |||
SearchHit searchHit = iterator.next(); | |||
try (XContentParser parser = createXContentParserFromRegistry(xContentRegistry, searchHit.getSourceRef())) { | |||
try ( | |||
XContentParser parser = createXContentParserFromRegistry(xContentRegistry.getRegistry(), searchHit.getSourceRef()) |
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.
Exactly as we want it. Someone (me? you?) should update the migration guide with this. Since it's part of create components should work nicely with this PR.
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 Ill take this as an action item
… client, settings. Spotless Apply Signed-off-by: Joshua Palis <jpalis@amazon.com>
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.
LGTM!
With the recent upgrade to snakeyaml 2.0. SafeConstructor can be replaced with
|
PR to fix the failure : opensearch-project/opensearch-sdk-java#504 |
Implementing create components, commented associated test classes for components until
SDKClusterSettings
support is added.Issues Resolved
Part of : opensearch-project/opensearch-sdk-java#499
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.