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] Migrate Create, Update, and Validate actions to extension #792

Merged
merged 26 commits into from
Feb 7, 2023

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Jan 28, 2023

Description

Migrates the IndexAnomalyDetectorTransportAction, ValidateAnomalyDetectorTransportAction, their Rest Handlers, and other supporting classes from plugin to extension.

Testing requires adding a document to an index which requires working around #5999 locally.

See SDK #372 for a summary of the changes that you'll see in the diff.

Issues Resolved

Fixes SDK #353.

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.

@dbwiddis dbwiddis force-pushed the rhlc branch 2 times, most recently from 8addf5a to 9f0f96e Compare February 1, 2023 22:52
@dbwiddis dbwiddis marked this pull request as ready for review February 1, 2023 22:52
@dbwiddis dbwiddis requested a review from a team February 1, 2023 22:52
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
@minalsha
Copy link

minalsha commented Feb 2, 2023

just a small suggestion: is it possible to break this huge PR into small PRs for quic review?

@dbwiddis dbwiddis requested a review from owaiskazi19 February 3, 2023 06:24
@dbwiddis
Copy link
Member Author

dbwiddis commented Feb 4, 2023

just a small suggestion: is it possible to break this huge PR into small PRs for quic review?

Unfortunately not without having a breaking change. However, there really aren't as many changes as you might think.

SDK #372 committed a migration guide which pretty much summarizes the diff here. I'll be a bit more specific if it helps. From top down on files:

  • build.gradle: had to suppress some jacoco exclusions due to test coverage and add dependency on rest high level client
  • ADExtension: add path to the new actions, and add getter for the rest client wrapper
  • ADPlugin: (UNUSED, Reference) mostly whitespace changes, setting client and clusterservice to null in objects whose class types have changed
  • Remaining classes mostly change Client to SDKRestClient, ClusterService to SDKClusterService, and change the imports for Request and Response objects. Exceptions follow:
    • Settings update consumers use a more efficient map approach
    • AnomalyDetectionIndices rollover request required new syntax for createRequest mapping and deleteIndex iteration
    • RestIndex and RestValidate DetectorActions are new format for extension rest actions, mostly self explanatory and indicated in migration doc
    • TransportActions doExecute made public
    • Test, a lot of things suppressed pending completion of createcomponents work next sprint

Signed-off-by: Daniel Widdis <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2023

Codecov Report

Merging #792 (7f68dc7) into feature/extensions (1ba8bc6) will decrease coverage by 2.22%.
The diff coverage is 13.13%.

📣 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     #792      +/-   ##
========================================================
- Coverage                 53.23%   51.02%   -2.22%     
+ Complexity                 2661     2576      -85     
========================================================
  Files                       291      291              
  Lines                     16099    16110      +11     
  Branches                   1689     1685       -4     
========================================================
- Hits                       8571     8220     -351     
- Misses                     6911     7307     +396     
+ Partials                    617      583      -34     
Flag Coverage Δ
plugin 51.02% <13.13%> (-2.22%) ⬇️

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%> (ø)
.../java/org/opensearch/ad/AnomalyDetectorPlugin.java 86.56% <ø> (-0.20%) ⬇️
...nsearch/ad/rest/AbstractAnomalyDetectorAction.java 0.00% <0.00%> (-100.00%) ⬇️
...search/ad/rest/RestIndexAnomalyDetectorAction.java 0.00% <0.00%> (-25.72%) ⬇️
...rch/ad/rest/RestValidateAnomalyDetectorAction.java 0.00% <0.00%> (ø)
...est/handler/IndexAnomalyDetectorActionHandler.java 100.00% <ø> (ø)
.../ad/rest/handler/ModelValidationActionHandler.java 0.00% <ø> (ø)
.../handler/ValidateAnomalyDetectorActionHandler.java 100.00% <ø> (ø)
...ansport/PreviewAnomalyDetectorTransportAction.java 76.92% <ø> (ø)
...nsport/ValidateAnomalyDetectorTransportAction.java 0.00% <0.00%> (ø)
... and 23 more

Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

Looks good overall with few changes and questions! Thanks for working on this @dbwiddis!

request,
RestStatus.OK,
new ValidateAnomalyDetectorResponse(issue).toXContent(JsonXContent.contentBuilder())
);
Copy link
Member

Choose a reason for hiding this comment

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

Can we move line 164-168 under sendAnomalyDetectorValidationParseResponse method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh, lines 164-168 are the replacement for sendAnomalyDetectorValidationParseResponse which should probably be deleted.

Unless by "under" you mean, change this to return sendAnomalyDetectorValidationParseResponse(issue) and move the 3 lines into that method? Sure.

src/main/java/org/opensearch/ad/util/ParseUtils.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/ad/util/ParseUtils.java Outdated Show resolved Hide resolved
Signed-off-by: Daniel Widdis <[email protected]>
Copy link
Member

@vibrantvarun vibrantvarun left a comment

Choose a reason for hiding this comment

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

Trying to understand why these many files are in jacoco exclusions?

@owaiskazi19 owaiskazi19 merged commit 318a992 into opensearch-project:feature/extensions Feb 7, 2023
@dbwiddis dbwiddis deleted the rhlc branch February 7, 2023 20:17
@dbwiddis
Copy link
Member Author

dbwiddis commented Feb 7, 2023

Trying to understand why these many files are in jacoco exclusions?

We've commented out a lot of the unit tests until the functionality is migrated. Goal is to re-enable them when migration is closer to complete (and we've uncommented/modified the tests)

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.

6 participants