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

[Feature/extensions] Add Action for Validate detector API call #732

Merged
merged 9 commits into from
Nov 18, 2022

Conversation

mloufra
Copy link

@mloufra mloufra commented Nov 16, 2022

Signed-off-by: Frank Lou [email protected]

Description

Create an action implementing ExtensionRestHandler with routes for the Post detector API calls.

Issues Resolved

Fixes opensearch-project/opensearch-sdk-java#221

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.

@mloufra mloufra requested a review from a team November 16, 2022 17:32
Signed-off-by: Frank Lou <[email protected]>

@Override
public List<Route> routes() {
return List.of(new Route(POST, "/detectors"));
Copy link
Member

Choose a reason for hiding this comment

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

Need the new routes here.

Copy link
Author

@mloufra mloufra Nov 17, 2022

Choose a reason for hiding this comment

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

@dbwiddis Do you means we need add new route HEAD here for checking configuration, since Validate detector is used to return the detector configuration has any issue that prevent Opensearch from creating the detector.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@mloufra there are 3 requests that will be handled, but only two routes: the wildcard {type} will handle two of them. The specific routes you need to map are in the issue for SDK #221.

Copy link
Author

@mloufra mloufra Nov 18, 2022

Choose a reason for hiding this comment

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

@dbwiddis @owaiskazi19 Thank you for your comments. For my understand, Validate detector having two option of validation. First one is focus on the detector configuration to find any issues which will block detector creation. Second one is focus on the source data to check the probability of complete model training. So we need two routes , one for detector POST _plugins/_anomaly_detection/detectors/_validate/detector, and one for model POST _plugins/_anomaly_detection/detectors/_validate/model.

Copy link
Member

@owaiskazi19 owaiskazi19 Nov 18, 2022

Choose a reason for hiding this comment

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

Based on the issue we will need 2 routes:

POST /detectors/_validate
POST /detectors/_validate/{type}

We are not covering the below route as a part of this issue

POST _plugins/_anomaly_detection/detectors/_validate/model

Copy link
Member

Choose a reason for hiding this comment

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

We are not covering the below route as a part of this issue

POST _plugins/_anomaly_detection/detectors/_validate/model

Actually, the {type} will include either detector or model that we'll extract with a param.get("type") as part of the logic later.

We could have listed the routes separately (_validate/detector and _validate/model) but using {type} parallels the existing plugin usage.

Signed-off-by: Frank Lou <[email protected]>
Signed-off-by: Frank Lou <[email protected]>
Signed-off-by: Frank Lou <[email protected]>
Signed-off-by: Frank Lou <[email protected]>
Signed-off-by: Frank Lou <[email protected]>
Signed-off-by: Frank Lou <[email protected]>
Signed-off-by: Frank Lou <[email protected]>
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!

@dbwiddis dbwiddis changed the title [Feature/extensions] Add Action for Post detector API call [Feature/extensions] Add Action for Validate detector API call Nov 18, 2022
@dbwiddis
Copy link
Member

@mloufra @owaiskazi19 build is failing due to changes in (merged) SDK PR #253 that have not yet been merged here in #725. Hopefully when #725 is merged this will pass.

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #732 (4ca48de) into feature/extensions (fa81e2e) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@                   Coverage Diff                    @@
##             feature/extensions     #732      +/-   ##
========================================================
- Coverage                 53.36%   53.33%   -0.04%     
  Complexity                 2659     2659              
========================================================
  Files                       290      291       +1     
  Lines                     16058    16068      +10     
  Branches                   1686     1687       +1     
========================================================
  Hits                       8570     8570              
- Misses                     6871     6881      +10     
  Partials                    617      617              
Flag Coverage Δ
plugin 53.33% <0.00%> (-0.04%) ⬇️

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/rest/RestValidateDetectorAction.java 0.00% <0.00%> (ø)

@dbwiddis dbwiddis merged commit 71010ca into opensearch-project:feature/extensions Nov 18, 2022
@mloufra mloufra deleted the post-detector branch November 21, 2022 18:43
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.

4 participants