Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Adding RestActions support for AD Search Rest API's #234

Merged
merged 5 commits into from
Sep 25, 2020

Conversation

saratvemulapalli
Copy link
Contributor

@saratvemulapalli saratvemulapalli commented Sep 17, 2020

Adding RestActions support for Search Rest API's.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Sep 17, 2020

Codecov Report

Merging #234 into master will decrease coverage by 0.28%.
The diff coverage is 96.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #234      +/-   ##
============================================
- Coverage     72.25%   71.97%   -0.29%     
- Complexity     1278     1284       +6     
============================================
  Files           139      143       +4     
  Lines          6045     5980      -65     
  Branches        469      469              
============================================
- Hits           4368     4304      -64     
+ Misses         1465     1462       -3     
- Partials        212      214       +2     
Flag Coverage Δ Complexity Δ
#cli 78.11% <ø> (-2.20%) 0.00 <ø> (ø)
#plugin 71.25% <96.55%> (+0.07%) 1284.00 <8.00> (+6.00)

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

Impacted Files Coverage Δ Complexity Δ
...stroforelasticsearch/ad/AnomalyDetectorPlugin.java 93.54% <ø> (ø) 10.00 <0.00> (ø)
...forelasticsearch/ad/rest/AbstractSearchAction.java 25.00% <66.66%> (+2.41%) 2.00 <0.00> (ø)
...earch/ad/rest/RestSearchAnomalyDetectorAction.java 100.00% <100.00%> (ø) 2.00 <0.00> (ø)
...csearch/ad/rest/RestSearchAnomalyResultAction.java 100.00% <100.00%> (ø) 2.00 <0.00> (ø)
...arch/ad/transport/SearchAnomalyDetectorAction.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...ransport/SearchAnomalyDetectorTransportAction.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...search/ad/transport/SearchAnomalyResultAction.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
.../transport/SearchAnomalyResultTransportAction.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...asticsearch/ad/cluster/ADClusterEventListener.java 88.00% <0.00%> (-6.00%) 13.00% <0.00%> (-2.00%)
cli/internal/gateway/ad/ad.go 60.43% <0.00%> (-4.95%) 0.00% <0.00%> (ø%)
... and 9 more

@saratvemulapalli saratvemulapalli added the enhancement New feature or request label Sep 17, 2020
Copy link

@skkosuri-amzn skkosuri-amzn left a comment

Choose a reason for hiding this comment

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

Plans to add UTs?


public class SearchAnomalyDetectorAction extends ActionType<SearchResponse> {
public static final SearchAnomalyDetectorAction INSTANCE = new SearchAnomalyDetectorAction();
public static final String NAME = "cluster:admin/ad/search/detector";

Choose a reason for hiding this comment

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

Lets use opendistro in the action name. For example: cluster:admin/opendistro/alerting/destinations/write

Also you might want to end with verb like: cluster:admin/opendistro/ad/detectors/search

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure that makes sense.
Will make changes.

import org.elasticsearch.transport.TransportService;


public class SearchAnomalyResultTransportAction extends HandledTransportAction<SearchRequest, SearchResponse> {
Copy link

@skkosuri-amzn skkosuri-amzn Sep 23, 2020

Choose a reason for hiding this comment

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

NP: your rest handler is called : RestSearchAnomalyDetectorAction, Would it be better to call transport handler as TransportSearchAnomalyDetectorAction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a good point. Existing Transport Actions were named that way, so I just followed them to be consistent.
Here are some examples:https://github.com/opendistro-for-elasticsearch/anomaly-detection/tree/master/src/main/java/com/amazon/opendistroforelasticsearch/ad/transport

@kaituo
Copy link
Member

kaituo commented Sep 23, 2020

Looks good overall. Will wait for your tests.

@saratvemulapalli
Copy link
Contributor Author

Plans to add UTs?

Thanks for taking a look. Yup will send them out today.

@saratvemulapalli
Copy link
Contributor Author

Looks good overall. Will wait for your tests.

Cool, thanks for taking a look. Yup I'll send out the tests today.

@saratvemulapalli saratvemulapalli merged commit 71923ff into master Sep 25, 2020
@saratvemulapalli saratvemulapalli deleted the fgac-search-apis branch September 25, 2020 19:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants