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] Migrates Search Detector Info action to extension #875

Conversation

owaiskazi19
Copy link
Member

@owaiskazi19 owaiskazi19 commented Apr 19, 2023

Description

This PR migrates SearchDetectorInfo to extension for GET request.

  • GET _extensions/_ad-detection/detectors/count
  • GET _plugins/_ad-detection/detectors/match

Issues Resolved

Closes opensearch-project/opensearch-sdk-java#675

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.

@owaiskazi19 owaiskazi19 requested review from a team, joshpalis and dbwiddis April 19, 2023 21:20
@owaiskazi19 owaiskazi19 force-pushed the search-detector-info branch from 747c0f9 to f63b170 Compare April 19, 2023 21:39
@owaiskazi19 owaiskazi19 changed the title Migrates Search Detector Info action to extension [Extensions] Migrates Search Detector Info action to extension Apr 20, 2023
@owaiskazi19 owaiskazi19 force-pushed the search-detector-info branch 7 times, most recently from d1f5b7e to 57b8ca4 Compare April 20, 2023 22:13
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Merging #875 (9964382) into feature/extensions (9ad715a) will decrease coverage by 0.11%.
The diff coverage is 3.33%.

❗ Current head 9964382 differs from pull request most recent head dc993ca. Consider uploading reports for the commit dc993ca to get more accurate results

📣 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

Impacted file tree graph

@@                   Coverage Diff                    @@
##             feature/extensions     #875      +/-   ##
========================================================
- Coverage                 35.02%   34.91%   -0.11%     
+ Complexity                 1896     1894       -2     
========================================================
  Files                       300      300              
  Lines                     17896    17915      +19     
  Branches                   1869     1869              
========================================================
- Hits                       6268     6255      -13     
- Misses                    11178    11206      +28     
- Partials                    450      454       +4     
Flag Coverage Δ
plugin 34.91% <3.33%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...va/org/opensearch/ad/AnomalyDetectorExtension.java 0.00% <0.00%> (ø)
...opensearch/ad/indices/AnomalyDetectionIndices.java 12.52% <0.00%> (ø)
...h/ad/rest/RestSearchAnomalyDetectorInfoAction.java 0.00% <0.00%> (ø)
...c/main/java/org/opensearch/ad/util/IndexUtils.java 2.38% <ø> (ø)
...port/SearchAnomalyDetectorInfoTransportAction.java 79.54% <100.00%> (-11.37%) ⬇️

... and 3 files with indirect coverage changes

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.

LGTM. One question and looks like a build.gradle conflict needs resolving.

build.gradle Outdated
'org.opensearch.ad.util.RestHandlerUtils'
'org.opensearch.ad.util.RestHandlerUtils',
'org.opensearch.ad.transport.SearchAnomalyDetectorInfoTransportAction.1',
'org.opensearch.ad.transport.SearchAnomalyDetectorInfoTransportAction.2'
Copy link
Member

Choose a reason for hiding this comment

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

For my own education, what are these .1 and .2? I don't see classes there. Can/should we use wildcards?

Copy link
Member Author

Choose a reason for hiding this comment

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

I went ahead and added wildcards for the rest as well.

@owaiskazi19 owaiskazi19 force-pushed the search-detector-info branch 2 times, most recently from 5f9b723 to c02c5a5 Compare April 24, 2023 18:56
@owaiskazi19
Copy link
Member Author

Needs #876 to be merged first for build to pass.

@joshpalis
Copy link
Member

@owaiskazi19 #876 has been merged. Seems that the build is failing for other instances of the same issue that I covered in that PR :

/home/runner/work/anomaly-detection/anomaly-detection/src/test/java/org/opensearch/ad/TestHelpers.java:1569: error: incompatible types: ImmutableOpenMap<String,IndexMetadata> cannot be converted to Map<String,IndexMetadata>
        Metadata metaData = Metadata.builder().indices(immutableOpenMap).build();
                                                       ^
/home/runner/work/anomaly-detection/anomaly-detection/src/test/java/org/opensearch/ad/transport/PreviewAnomalyDetectorTransportActionTests.java:127: error: incompatible types: ImmutableOpenMap<String,IndexMetadata> cannot be converted to Map<String,IndexMetadata>
        ClusterState clusterState = ClusterState.builder(clusterName).metadata(Metadata.builder().indices(indices).build()).build();
                                                                                                          ^
/home/runner/work/anomaly-detection/anomaly-detection/src/test/java/org/opensearch/ad/transport/IndexAnomalyDetectorTransportActionTests.java:117: error: incompatible types: ImmutableOpenMap<String,IndexMetadata> cannot be converted to Map<String,IndexMetadata>
        ClusterState clusterState = ClusterState.builder(clusterName).metadata(Metadata.builder().indices(indices).build()).build();

Simple fix is to convert the ImmutableOpenMap just to Map

Copy link
Member

@joshpalis joshpalis left a comment

Choose a reason for hiding this comment

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

LGTM other than the failing tests

Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

Thanks @owaiskazi19, changes look good to me!


public RestSearchAnomalyDetectorInfoAction() {}
public RestSearchAnomalyDetectorInfoAction(ExtensionsRunner extensionsRunner, SDKRestClient client) {
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker for this PR: Curious if there is a better way to get information from extensions runner.
We just dont need to pass this across the extension.

Copy link
Member Author

@owaiskazi19 owaiskazi19 Apr 27, 2023

Choose a reason for hiding this comment

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

That's a good question. @dbwiddis /@joshpalis do you think we can add ExtensionsRunner in createComponent and inject from there?

Copy link
Member

Choose a reason for hiding this comment

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

ExtensionsRunner is already bound to guice here. I think what we need is to enable injection for rest actions so that we don't need to pass objects around in AD Extension

@owaiskazi19 owaiskazi19 force-pushed the search-detector-info branch from c02c5a5 to 57795f3 Compare April 27, 2023 19:05
@owaiskazi19 owaiskazi19 force-pushed the search-detector-info branch from 9964382 to 811a202 Compare April 27, 2023 19:44
@owaiskazi19 owaiskazi19 force-pushed the search-detector-info branch from 811a202 to dc993ca Compare April 27, 2023 19:48
@owaiskazi19 owaiskazi19 merged commit cf70e72 into opensearch-project:feature/extensions Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants