[Feature/extensions] Removed Job Scheduler dependencies from AD by owaiskazi19 · Pull Request #647 · opensearch-project/anomaly-detection

owaiskazi19 · 2022-08-30T23:51:04Z

Description

This PR:

Removes Job Scheduler dependency from build.gradle dependencies
Removes Job Scheduler dependency from the codebase
Removes Job Scheduler from extended plugin
Removes all features except Creating Detector from AD
Removes test related to other features
Fixes running GHA build on feature branches
Publish OpenSearch feature/extensions branch in GHA
Used custom distribution url to build AD
Deleted DCO check since we have the DCO app now

Before running the feature/extensions branch of AD:

The feature/extensions branch of OpenSearch with 3.0.0-SNAPSHOT should be published to mavenLocal.
Assemble OpenSearch feature/extensions branch
Run AD with the custom distribution url

./gradlew build -PcustomDistributionUrl=/home/ubuntu/OpenSearch/distribution/archives/linux-tar/build/distributions/opensearch-min-3.0.0-SNAPSHOT-linux-x64.tar.gz

Issues Resolved

opensearch-project/opensearch-sdk-java#113

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.

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

saratvemulapalli · 2022-09-01T16:27:58Z

@owaiskazi19 is this ready for review?

owaiskazi19 · 2022-09-01T16:55:42Z

@owaiskazi19 is this ready for review?

Still working on the build errors.

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

codecov-commenter · 2022-09-02T21:26:04Z

Codecov Report

❗ No coverage uploaded for pull request base (feature/extensions@572fd8d). Click here to learn what that means.
The diff coverage is n/a.

@@                  Coverage Diff                  @@
##             feature/extensions     #647   +/-   ##
=====================================================
  Coverage                      ?   53.80%           
  Complexity                    ?     2679           
=====================================================
  Files                         ?      288           
  Lines                         ?    16004           
  Branches                      ?     1703           
=====================================================
  Hits                          ?     8611           
  Misses                        ?     6768           
  Partials                      ?      625

Flag	Coverage Δ
plugin	`53.80% <0.00%> (?)`

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

minalsha · 2022-09-08T16:31:41Z

@saratvemulapalli @dbwiddis this PR is ready for review. Please help reviewing this to unblock other dependent workitems. Thanks

saratvemulapalli

Overall the changes look good to me.
Just put in tags for the commented piece of code, it will help when we get back to AD main merging to feature/extensions

saratvemulapalli · 2022-09-07T18:08:59Z

build.gradle

@@ -212,93 +212,93 @@ test {

 // Commented this code until Job Scheduler is published to https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/ with OpenSearch 3.0.0-SNAPSHOT



Could you add a tag for every piece of commented code which should help understand why we have removed it?

Same comment for rest of the code changes.

saratvemulapalli · 2022-09-08T17:53:46Z

build.gradle

    description 'OpenSearch anomaly detector plugin'
    classname 'org.opensearch.ad.AnomalyDetectorPlugin'
-    extendedPlugins = ['lang-painless', 'opensearch-job-scheduler']
+    extendedPlugins = ['lang-painless']


dbwiddis

Approved with suggestions.

dbwiddis · 2022-09-08T20:52:57Z

src/main/java/org/opensearch/ad/AnomalyDetectorPlugin.java

+    // XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser);
+    // return AnomalyDetectorJob.parse(parser);
+    // };
+    // }


FYI, mentioning this once but applies for most of the files. Using /*- at the start of a block comment preserves the formatting of everything until */ and reduces the diff noise. Not sure whether that makes it easier or harder to review, but something to consider in the future when commenting out large blocks.

dbwiddis · 2022-09-08T20:57:59Z

src/main/java/org/opensearch/ad/rest/handler/IndexAnomalyDetectorJobActionHandler.java

-    }
-
-}
+/// *


This looks like we commented out the entire file (including the copyright header). Some q's:

Will that break license header checks?

Pretty sure we should keep at least the package definition and class name. Surprised this compiles otherwise.

If this is totally unneeded, is it ever actually called by anything? Why not delete it or give it a name like "UnusedIndexAnomaly...."

No, the build passed

^^

The reason I didn't delete it because we will rebase feature/extensions with the latest main later and these files will be needed. Even, I thought of having a different name for these files but since they import dependencies from Job Scheduler it fails during compile.

dbwiddis · 2022-09-08T20:59:45Z

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

-        return profileList.stream().map(profile -> profile.getName()).collect(Collectors.toSet());
-    }
-}
+/// *


Same questions as before re: commenting out an entire file vs. just leaving it (possibly renaming) and not using it.

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

owaiskazi19 added 2 commits August 30, 2022 15:20

Removed JS from AD

bacf786

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Removed JS from extended plugin

e2f24c7

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

owaiskazi19 requested a review from a team August 30, 2022 23:51

owaiskazi19 changed the title ~~Removed Job Scheduler dependencies from AD~~ [Feature/extensions] Removed Job Scheduler dependencies from AD Aug 30, 2022

owaiskazi19 requested review from dbwiddis and saratvemulapalli August 30, 2022 23:51

owaiskazi19 added 4 commits August 31, 2022 00:13

Fix gradle build GHA

0cb58ea

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Publish OpenSearch to maven local

25e29b8

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Fixed GHA synatx error

e1c36a4

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Fixed GHA syntax error

1d8f7a6

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

owaiskazi19 mentioned this pull request Aug 31, 2022

[Feature/extensions] Build AD with OpenSearch 3.0 #645

Merged

owaiskazi19 added 17 commits September 1, 2022 13:58

Run Integ Test and fix build errors using custom distribution url

fdb133d

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Test: finding the location on CI

c389b08

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Added custom distribution url

1c18787

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Fix relative path

a1c5356

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Test: Build path

a8fe0fa

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Test: List directories

32819f1

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Added pwd

a7c2ded

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Fixed syntax error

1b05517

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Added pwd

0a6c0b6

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Added path for OpenSearch

ea1a53b

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Combined steps for OpenSearch

b5070e3

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Fixed path

2b870c9

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Fixed root path

0987538

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Find tar

b4f6da6

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Change dir

4a27328

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Fixed OpenSearch path

89908ba

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Change location of os

669f642

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

owaiskazi19 added 6 commits September 1, 2022 19:27

Added flag clean false

c5e250a

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Rerun build

8dd33d5

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Copy snapshot to tmp

e18b8fe

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Publish OpenSearch to maven local

86dbbe1

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Commenting BWC tests

a9ab6bc

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

Removed pwd and ls

f843936

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

saratvemulapalli reviewed Sep 8, 2022

View reviewed changes

dbwiddis approved these changes Sep 8, 2022

View reviewed changes

Added tag for commented code

75568f3

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

owaiskazi19 force-pushed the remove-js branch from d4e193b to 75568f3 Compare September 8, 2022 21:56

Deleted DCO check

864129b

Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>

owaiskazi19 requested a review from saratvemulapalli September 8, 2022 22:02

dbwiddis approved these changes Sep 9, 2022

View reviewed changes

saratvemulapalli approved these changes Sep 9, 2022

View reviewed changes

saratvemulapalli merged commit b176ae5 into opensearch-project:feature/extensions Sep 9, 2022

		@@ -212,93 +212,93 @@ test {

		// Commented this code until Job Scheduler is published to https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/ with OpenSearch 3.0.0-SNAPSHOT

Conversation

owaiskazi19 commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Uh oh!

saratvemulapalli commented Sep 1, 2022

Uh oh!

owaiskazi19 commented Sep 1, 2022

Uh oh!

codecov-commenter commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

minalsha commented Sep 8, 2022

Uh oh!

saratvemulapalli left a comment

Choose a reason for hiding this comment

Uh oh!

saratvemulapalli Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

saratvemulapalli Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

owaiskazi19 Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

saratvemulapalli Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

dbwiddis left a comment

Choose a reason for hiding this comment

Uh oh!

dbwiddis Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

dbwiddis Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

owaiskazi19 Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbwiddis Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

owaiskazi19 commented Aug 30, 2022 •

edited

Loading

codecov-commenter commented Sep 2, 2022 •

edited

Loading

owaiskazi19 Sep 8, 2022 •

edited

Loading