[Extensions] Migrates Search, SearchResults and SearchADTasks action to extensions #849

owaiskazi19 · 2023-03-29T23:15:13Z

Description

This PR migrates SearchDetector, SearchResultAnomalyDetector and SearchADTasks to extension for GET and POST request.

Tested Search Detector for GET and POST.

GET /_extensions/_ad-extension/detectors/_search
POST /_extensions/_ad-extension/detectors/_search

Tested Search Anomaly Detector Result (This is without Migrate AnomalyResultAction/TransportAction to SDK using the SDKClient opensearch-sdk-java#626)

POST /_extensions/_ad-extension/detectors/results/_search/
POST /_extensions/_ad-extension/detectors/results/_search/<custom_result_index>
POST /_extensions/_ad-extension/detectors/results/_search/<custom_result_index>?only_query_custom_result_index=false
POST /_extensions/_ad-extension/detectors/results/_search/<custom_result_index>?only_query_custom_result_index=true

Tested Search AD Tasks

GET _plugins/_anomaly_detection/detectors/tasks/_search
POST _plugins/_anomaly_detection/detectors/tasks/_search

Issues Resolved

Part of opensearch-project/opensearch-sdk-java#376, opensearch-project/opensearch-sdk-java#377 and opensearch-project/opensearch-sdk-java#380

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 · 2023-03-29T23:17:07Z

src/main/java/org/opensearch/ad/rest/AbstractSearchAction.java

-            @Override
-            public RestResponse buildResponse(SearchResponse response) throws Exception {
+    protected ExtensionRestResponse search(ExtensionRestRequest request, SearchResponse response) throws IOException {
+//        return new RestResponseListener<SearchResponse>(channel) {


@dbwiddis need some suggestion here. Do we require a Transport API for RestResponseListener or CompletableFuture should be enough?

@owaiskazi19 I'm not sure I understand the question here. Is this a situation where we have a long-running request?

My initial thoughts are "wait some reasonable timeout for a fast request, if it's done send the search response, if not, send a response containing information to access the results later".

Does that mean CompletableFuture should be enough to handle the RestResponseListener here?

codecov-commenter · 2023-04-08T00:59:45Z

Codecov Report

Merging #849 (42e9dcc) into feature/extensions (8b31c13) will decrease coverage by 0.05%.
The diff coverage is 1.66%.

📣 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     #849      +/-   ##
========================================================
- Coverage                 35.96%   35.91%   -0.05%     
- Complexity                 1916     1917       +1     
========================================================
  Files                       299      299              
  Lines                     17615    17645      +30     
  Branches                   1862     1861       -1     
========================================================
+ Hits                       6335     6337       +2     
- Misses                    10825    10854      +29     
+ Partials                    455      454       -1

Flag	Coverage Δ
plugin	`35.91% <1.66%> (-0.05%)`	⬇️

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%> (ø)`
...a/org/opensearch/ad/rest/AbstractSearchAction.java	`0.00% <0.00%> (ø)`
...rg/opensearch/ad/rest/RestSearchADTasksAction.java	`0.00% <ø> (ø)`
...earch/ad/rest/RestSearchAnomalyDetectorAction.java	`0.00% <ø> (ø)`
...nsearch/ad/rest/RestSearchAnomalyResultAction.java	`0.00% <0.00%> (ø)`
...rch/ad/transport/SearchADTasksTransportAction.java	`0.00% <0.00%> (ø)`
...ransport/SearchAnomalyDetectorTransportAction.java	`0.00% <0.00%> (ø)`
.../transport/SearchAnomalyResultTransportAction.java	`0.00% <0.00%> (ø)`
...ensearch/ad/transport/handler/ADSearchHandler.java	`69.56% <100.00%> (ø)`

... and 1 file with indirect coverage changes

joshpalis · 2023-04-14T17:52:41Z

src/main/java/org/opensearch/ad/transport/SearchAnomalyDetectorAction.java

 public class SearchAnomalyDetectorAction extends ActionType<SearchResponse> {
    // External Action which used for public facing RestAPIs.
    public static final String NAME = CommonValue.EXTERNAL_ACTION_PREFIX + "detector/search";
    public static final SearchAnomalyDetectorAction INSTANCE = new SearchAnomalyDetectorAction();

+    @Inject


Im curious about the purpose of adding an inject annotation to a no arg constructor

Good call out. Removed it.

joshpalis · 2023-04-14T17:53:00Z

src/main/java/org/opensearch/ad/transport/SearchAnomalyResultAction.java

 public class SearchAnomalyResultAction extends ActionType<SearchResponse> {
    // External Action which used for public facing RestAPIs.
    public static final String NAME = CommonValue.EXTERNAL_ACTION_PREFIX + "result/search";
    public static final SearchAnomalyResultAction INSTANCE = new SearchAnomalyResultAction();

+    @Inject


joshpalis · 2023-04-14T17:56:21Z

src/main/java/org/opensearch/ad/transport/handler/ADSearchHandler.java

    private volatile Boolean filterEnabled;

-    public ADSearchHandler(Settings settings, ClusterService clusterService, Client client) {
+    @Inject


I see that we're returning this object as part of create components, there's no need to add this inject annotation here. I may be wrong though, for my own learnings, what's the reason for this change?

You are right. Removed it. I'm a beginner in Guice and learning it step by step. 😄

joshpalis

Other than a minor nit, looks good to me

joshpalis · 2023-04-17T17:19:48Z

src/test/java/org/opensearch/ad/transport/handler/ADSearchHandlerTests.java

+        Settings settings = Settings.EMPTY;
+        List<Setting<?>> settingsList = List.of(AnomalyDetectorSettings.MAX_ENTITIES_FOR_PREVIEW, AnomalyDetectorSettings.PAGE_SIZE);
+        clusterService = mock(SDKClusterService.class);
+        ;


seems like a stray semicolon :)

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 commented Mar 29, 2023

View reviewed changes

owaiskazi19 force-pushed the search-detectors branch 4 times, most recently from 6d065ec to fd57c8c Compare April 5, 2023 19:00

owaiskazi19 force-pushed the search-detectors branch 3 times, most recently from f57529f to bc315cd Compare April 8, 2023 00:53

owaiskazi19 force-pushed the search-detectors branch from bc315cd to 34edec8 Compare April 12, 2023 21:28

owaiskazi19 marked this pull request as ready for review April 12, 2023 22:06

owaiskazi19 requested a review from a team April 12, 2023 22:06

joshpalis self-requested a review April 14, 2023 17:44

joshpalis reviewed Apr 14, 2023

View reviewed changes

owaiskazi19 force-pushed the search-detectors branch 4 times, most recently from e465480 to 64f0abb Compare April 15, 2023 00:27

owaiskazi19 requested review from dbwiddis and joshpalis April 15, 2023 00:32

joshpalis approved these changes Apr 17, 2023

View reviewed changes

owaiskazi19 force-pushed the search-detectors branch from 64f0abb to 42e9dcc Compare April 17, 2023 18:07

owaiskazi19 added 5 commits April 18, 2023 10:41

Migrates Search, SearchResults and SearchADTasks action to extensions

3f058fe

Signed-off-by: Owais Kazi <[email protected]>

Added POST method, reformating and working test

e1a55c2

Signed-off-by: Owais Kazi <[email protected]>

Add routes for search result detector

599be07

Signed-off-by: Owais Kazi <[email protected]>

Changed the return type for aggregation

9150e7c

Signed-off-by: Owais Kazi <[email protected]>

Integrated replacedRouteHandlers with Search Detectors

2ce8222

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the search-detectors branch from 42e9dcc to 2ce8222 Compare April 18, 2023 18:43

owaiskazi19 merged commit 86b1084 into opensearch-project:feature/extensions Apr 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Extensions] Migrates Search, SearchResults and SearchADTasks action to extensions #849

[Extensions] Migrates Search, SearchResults and SearchADTasks action to extensions #849

owaiskazi19 commented Mar 29, 2023 •

edited

Loading

owaiskazi19 Mar 29, 2023

dbwiddis Apr 5, 2023

owaiskazi19 Apr 15, 2023

codecov-commenter commented Apr 8, 2023 •

edited

Loading

joshpalis Apr 14, 2023

owaiskazi19 Apr 15, 2023

joshpalis Apr 14, 2023

joshpalis Apr 14, 2023

owaiskazi19 Apr 15, 2023

joshpalis left a comment

joshpalis Apr 17, 2023

[Extensions] Migrates Search, SearchResults and SearchADTasks action to extensions #849

[Extensions] Migrates Search, SearchResults and SearchADTasks action to extensions #849

Conversation

owaiskazi19 commented Mar 29, 2023 • edited Loading

Description

Issues Resolved

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 8, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshpalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owaiskazi19 commented Mar 29, 2023 •

edited

Loading

codecov-commenter commented Apr 8, 2023 •

edited

Loading