update the AD settings for opensearch. #47

spbjss · 2021-05-17T23:48:55Z

Description

Rename plugin settings from opendistro to opensearch.

Issues Resolved

Check List

[Y] Commits are signed per the DCO using --signoff

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: Alex <[email protected]>

kaituo · 2021-05-18T00:00:40Z

src/test/java/org/opensearch/ad/transport/ADBatchAnomalyResultTransportActionTests.java

@@ -137,7 +137,7 @@ public void testHistoricalDetectorWithValidDateRange() throws IOException, Inter
        DetectionDateRange dateRange = new DetectionDateRange(startTime, endTime);
        ADBatchAnomalyResultRequest request = adBatchAnomalyResultRequest(dateRange);
        client().execute(ADBatchAnomalyResultAction.INSTANCE, request).actionGet(5000);
-        Thread.sleep(10000);
+        Thread.sleep(20000);


This will make our tests slower and slower when adding more tests. We want to avoid even though the execute finishes quickly and we still sleep 20 seconds. You can use a CountDownLatch to wait while the client().execute is running. See an example in EntityProfileRunnerTests.stateTestTemplate

Thank you very much for the suggestion!
I have checked with Yaliang, he is still working on the batching change and need more time to optimize the unit tests.
I'm planning to update these unit tests after Yaliang finished his PRs.

I will fix UTs related with AD batch tasks after all changes ready. I'm ok if @spbjss willing to put some effort to fix it now or leave it to later to avoid duplicate effort.

sounds good. Will approve the PR once you get the build issue figured out. Please let me know.

dblock · 2021-05-18T19:04:17Z

src/main/java/org/opensearch/ad/settings/AnomalyDetectorSettings.java

@@ -40,8 +40,8 @@ private AnomalyDetectorSettings() {}

    public static final Setting<Integer> MAX_SINGLE_ENTITY_ANOMALY_DETECTORS = Setting
        .intSetting(
-            "opendistro.anomaly_detection.max_anomaly_detectors",
-            1000,
+            "opensearch.anomaly_detection.max_anomaly_detectors",


I know @saratvemulapalli has been saying that we would want these to be plugins. just to avoid renaming again :) Care to comment?

I think it was miss on our side, we haven't updated the guidance. I would prefer to have plugins instead of opensearch.
Can we update the doc: https://github.com/opensearch-project/opensearch-plugins/blob/main/CONVENTIONS.md#settings

Also let folks working on JobScheduler to take care of it as well.

Do you mean we should change "opensearch.anomaly_detection.xxx" to "plugins.anomaly_detection.xxx"?

Sure, I will make that change.
Thank you for the comments!

Yes please. Thanks Alex!

And why would we not make this anomaly_detection.max_anomaly_detectors? That's what k-nn is doing, for example.

Definitely that works too. But I would love to have consistency across different pieces in the plugins.
And, we have decided to go with that as we have already discussed in the public:
opensearch-project/opensearch-plugins#13
https://discuss.opendistrocommunity.dev/t/thoughts-on-having-opensearch-identifier-in-all-things-plugins/5855

I am not opposed to either way but since we have taken a decision in the public, I would prefer to stick with it unless we see strong contentions to why that doesnt work. What do you think?

A-OK by me to include plugins. opensearch-project/opensearch-plugins#31

Haha oncall :/, thanks for the change!

Thank you very much for the input!
I have submitted a commits to update "opensearch" to "plugins"

Signed-off-by: Alex <[email protected]>

latest code which is requred to support the setting change. Signed-off-by: Alex <[email protected]>

ylwu-amzn · 2021-05-19T01:54:46Z

@spbjss Changed to main branch for OpenSearch in CI (#50). Can you merge main branch to check if it can pass the github CI workflow?

ylwu-amzn · 2021-05-19T06:33:13Z

From CI workflow log, the compile can pass, but integTest failed, error message

=== Standard output of node `node{::integTest-0}` ===
> Task :integTest FAILED

»    ↓ errors and warnings from /home/runner/work/anomaly-detection/anomaly-detection/build/testclusters/integTest-0/logs/opensearch.stdout.log ↓
» ERROR][o.o.b.OpenSearchUncaughtExceptionHandler] [integTest-0] fatal error in thread [main], exiting
»  java.lang.NoSuchMethodError: 'org.opensearch.common.settings.Setting org.opensearch.common.settings.Setting.positiveTimeSetting(java.lang.String, org.opensearch.common.settings.Setting, org.opensearch.common.settings.Setting$Property[])'
»  	at org.opensearch.ad.settings.AnomalyDetectorSettings.<clinit>(AnomalyDetectorSettings.java:72) ~[?:?]
»  	at org.opensearch.ad.AnomalyDetectorPlugin.getSettings(AnomalyDetectorPlugin.java:639) ~[?:?]
»  	at org.opensearch.plugins.PluginsService.lambda$getPluginSettings$0(PluginsService.java:107) ~[opensearch-1.0.0-beta1.jar:1.0.0-beta1]
»  	at java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:271) ~[?:?]
»  	at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625) ~[?:?]

The integTest will always use opensearch-1.0.0-beta1 to build test cluster for integTest. @dblock Any suggestion how to unblock the integTest? Should we build beta2 or 1.x to include the code change of OpenSearch PR 665? This PR removed final TimeValue minValue from input param list of function Setting#positiveTimeSetting. So we can't use beta1 to build test cluster to run latest code.

ylwu-amzn · 2021-05-19T07:39:19Z

Several options to unblock the integTest,

Use old setting function with some redundancy code: ylwu-amzn@3616080.
Build new artifact with latest Setting class for integTest.
Ignore the integTest failure, just merge it directly. We may have no way to run integTest until we have a new OpenSearch artifact ready.

saratvemulapalli · 2021-05-19T15:25:02Z

From CI workflow log, the compile can pass, but integTest failed, error message

=== Standard output of node `node{::integTest-0}` ===
> Task :integTest FAILED

»    ↓ errors and warnings from /home/runner/work/anomaly-detection/anomaly-detection/build/testclusters/integTest-0/logs/opensearch.stdout.log ↓
» ERROR][o.o.b.OpenSearchUncaughtExceptionHandler] [integTest-0] fatal error in thread [main], exiting
»  java.lang.NoSuchMethodError: 'org.opensearch.common.settings.Setting org.opensearch.common.settings.Setting.positiveTimeSetting(java.lang.String, org.opensearch.common.settings.Setting, org.opensearch.common.settings.Setting$Property[])'
»  	at org.opensearch.ad.settings.AnomalyDetectorSettings.<clinit>(AnomalyDetectorSettings.java:72) ~[?:?]
»  	at org.opensearch.ad.AnomalyDetectorPlugin.getSettings(AnomalyDetectorPlugin.java:639) ~[?:?]
»  	at org.opensearch.plugins.PluginsService.lambda$getPluginSettings$0(PluginsService.java:107) ~[opensearch-1.0.0-beta1.jar:1.0.0-beta1]
»  	at java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:271) ~[?:?]
»  	at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625) ~[?:?]

The integTest will always use opensearch-1.0.0-beta1 to build test cluster for integTest. @dblock Any suggestion how to unblock the integTest? Should we build beta2 or 1.x to include the code change of OpenSearch PR 665? This PR removed final TimeValue minValue from input param list of function Setting#positiveTimeSetting. So we can't use beta1 to build test cluster to run latest code.

@ylwu-amzn @spbjss There were code changes to make the backward compatibility happen in OpenSearch. The CI should run a build out of 1.x branch of OpenSearch.
The integTests will be unblocked once infra team builds the interim build which is due tomorrow.
Please see Job Scheduler PR: opensearch-project/job-scheduler@6ca46bf

@dblock action items for us:

Work with infra team to get the build artifacts our for 1.x (may be the interim build?)
Update this in the plugins repository to reflect plugins to build from release branch (1.x for now).
Finalize the branching strategy and help plugins to follow them.

dblock · 2021-05-19T17:45:04Z

@ylwu-amzn @spbjss would you please followup with infra team on (1)? cc: @bbarani

saratvemulapalli · 2021-05-19T17:47:37Z

@ylwu-amzn @spbjss would you please followup with infra team on (1)? cc: @bbarani

Also can we open an issue in opensearch-build to track this?

ylwu-amzn · 2021-05-19T18:58:44Z

@ylwu-amzn @spbjss would you please followup with infra team on (1)? cc: @bbarani

Also can we open an issue in opensearch-build to track this?

Talked with Peter @peterzhuamazon , we will have a new tarball tomorrow. Do we need to change the tarball link used in integTest ? Who will change it? @dblock @saratvemulapalli

… 1.0.0-beta1 to 1.x 2. Update the code import from opendistro to opensearch 3. Update the CI workflow script to apply the dependencis version change. Signed-off-by: Alex <[email protected]>

Signed-off-by: Alex <[email protected]>

dblock · 2021-05-20T15:30:23Z

@ylwu-amzn @spbjss would you please followup with infra team on (1)? cc: @bbarani

Also can we open an issue in opensearch-build to track this?

Talked with Peter @peterzhuamazon , we will have a new tarball tomorrow. Do we need to change the tarball link used in integTest ? Who will change it? @dblock @saratvemulapalli

As a general rule of thumb when we need something we should be opening issues. I got pinged by several people about this, so opened opensearch-project/opensearch-build#39 and just point them to it.

dblock · 2021-05-20T15:31:27Z

Do we need to change the tarball link used in integTest ? Who will change it? @dblock @saratvemulapalli

@spbjss who opened this PR wants it to get to green so I imagine he would ;)

saratvemulapalli · 2021-05-20T15:35:11Z

Do we need to change the tarball link used in integTest ? Who will change it? @dblock @saratvemulapalli

@spbjss who opened this PR wants it to get to green so I imagine he would ;)

Haha I think the link should work, we have agreed upon a format between Infra and Opensearch, Distribution Downloader should be able to handle that.
https://github.com/opensearch-project/OpenSearch/blob/0e9f74e35f1255cb9ec45be3d8960aad195a9f6e/buildSrc/src/main/java/org/opensearch/gradle/DistributionDownloadPlugin.java#L159

Signed-off-by: Alex <[email protected]>

spbjss · 2021-05-20T21:57:59Z

Do we need to change the tarball link used in integTest ? Who will change it? @dblock @saratvemulapalli

@spbjss who opened this PR wants it to get to green so I imagine he would ;)

Haha I think the link should work, we have agreed upon a format between Infra and Opensearch, Distribution Downloader should be able to handle that.
https://github.com/opensearch-project/OpenSearch/blob/0e9f74e35f1255cb9ec45be3d8960aad195a9f6e/buildSrc/src/main/java/org/opensearch/gradle/DistributionDownloadPlugin.java#L159

I could change the tar url of the integration test to "https://artifacts.opensearch.org/releases/core/opensearch/1.0.0-rc1/opensearch-1.0.0-rc1-linux-x64.tar.gz", waiting for infrastructure team to build the rc1 artifact.

codecov-commenter · 2021-05-21T01:50:52Z

Codecov Report

Merging #47 (4325929) into main (0f3b118) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main      #47      +/-   ##
============================================
+ Coverage     79.28%   79.41%   +0.12%     
- Complexity     2691     2692       +1     
============================================
  Files           242      243       +1     
  Lines         11065    11123      +58     
  Branches       1012     1012              
============================================
+ Hits           8773     8833      +60     
+ Misses         1878     1876       -2     
  Partials        414      414

Flag	Coverage Δ	Complexity Δ
plugin	`79.41% <100.00%> (+0.12%)`	`2692.00 <2.00> (+1.00)`

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

Impacted Files	Coverage Δ	Complexity Δ
...va/org/opensearch/ad/AnomalyDetectorJobRunner.java	`75.00% <ø> (ø)`	`36.00 <0.00> (ø)`
.../java/org/opensearch/ad/AnomalyDetectorPlugin.java	`95.38% <ø> (ø)`	`10.00 <0.00> (ø)`
src/main/java/org/opensearch/ad/model/ADTask.java	`98.12% <ø> (+1.56%)`	`68.00 <0.00> (+2.00)`
.../java/org/opensearch/ad/model/AnomalyDetector.java	`95.59% <ø> (ø)`	`91.00 <0.00> (ø)`
...va/org/opensearch/ad/model/AnomalyDetectorJob.java	`60.68% <ø> (ø)`	`26.00 <0.00> (ø)`
...in/java/org/opensearch/ad/model/AnomalyResult.java	`99.40% <ø> (ø)`	`58.00 <0.00> (ø)`
...est/handler/IndexAnomalyDetectorActionHandler.java	`49.39% <ø> (ø)`	`32.00 <0.00> (ø)`
.../handler/IndexAnomalyDetectorJobActionHandler.java	`60.99% <ø> (ø)`	`16.00 <0.00> (ø)`
...ain/java/org/opensearch/ad/task/ADTaskManager.java	`73.21% <ø> (ø)`	`82.00 <0.00> (ø)`
...d/transport/AnomalyDetectorJobTransportAction.java	`88.57% <ø> (ø)`	`5.00 <0.00> (ø)`
... and 15 more

dblock

Good stuff.

saratvemulapalli

Looks great!

saratvemulapalli · 2021-05-21T13:51:49Z

.github/workflows/CI.yml

@@ -33,37 +33,37 @@ jobs:
        with:
          repository: 'opensearch-project/OpenSearch'
          path: OpenSearch
-          ref: 'main'
+          ref: '1.x'


saratvemulapalli · 2021-05-21T13:53:47Z

.github/workflows/CI.yml


      # dependencies: common-utils
      - name: Checkout common-utils
        uses: actions/checkout@v2
        with:
-          ref: '1.0.0-beta1'
+          ref: 'main' # TODO: update to the right branch name once it's ready. e.g. 1.x


We should add instructions for our branching strategy for plugins in the plugins repo. @dblock
I can take care of it.

Issue: opensearch-project/opensearch-plugins#35

update the AD settings for opensearch.

7996ed5

Signed-off-by: Alex <[email protected]>

spbjss requested review from kaituo, ylwu-amzn and ohltyler May 17, 2021 23:48

kaituo reviewed May 18, 2021

View reviewed changes

dblock reviewed May 18, 2021

View reviewed changes

dblock mentioned this pull request May 18, 2021

Use plugins. for settings. opensearch-project/opensearch-plugins#31

Merged

1 task

Alex added 3 commits May 18, 2021 15:06

Update settings name prefix from opensearch to plugins.

2077548

Signed-off-by: Alex <[email protected]>

Update the version of checkstyle to keep sync with opensearch.

dfbbea6

Signed-off-by: Alex <[email protected]>

Update the Opensearch from 1.0.0-beta1 to the main branch to take the

9e398fd

latest code which is requred to support the setting change. Signed-off-by: Alex <[email protected]>

spbjss and others added 2 commits May 18, 2021 19:22

Merge branch 'opensearch-project:main' into main

eb4b0ab

Merge branch 'main' of https://github.com/spbjss/anomaly-detection

6e5c9e9

Alex added 4 commits May 19, 2021 13:04

1. Update the opensearch, common-utils and job-scheduler version from…

8fec893

… 1.0.0-beta1 to 1.x 2. Update the code import from opendistro to opensearch 3. Update the CI workflow script to apply the dependencis version change. Signed-off-by: Alex <[email protected]>

Update the opensearch version from 1.0.0 to 1.0.0-rc1

40b4d99

Signed-off-by: Alex <[email protected]>

Update the version informations.

45ffd32

Signed-off-by: Alex <[email protected]>

Update the rc1 related version config

5d368a6

Signed-off-by: Alex <[email protected]>

Update checkstyle version to 8.29 to follow opensearch 1.x

4325929

Signed-off-by: Alex <[email protected]>

spbjss requested a review from kaituo May 21, 2021 02:30

spbjss requested review from saratvemulapalli and dblock May 21, 2021 02:30

dblock approved these changes May 21, 2021

View reviewed changes

saratvemulapalli approved these changes May 21, 2021

View reviewed changes

spbjss merged commit ddf6b0e into opensearch-project:main May 21, 2021

ylwu-amzn mentioned this pull request Jun 22, 2021

Rename dependency namespaces #45

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update the AD settings for opensearch. #47

update the AD settings for opensearch. #47

spbjss commented May 17, 2021

kaituo May 18, 2021

spbjss May 18, 2021

ylwu-amzn May 18, 2021

kaituo May 18, 2021

dblock May 18, 2021

saratvemulapalli May 18, 2021

spbjss May 18, 2021

saratvemulapalli May 18, 2021

dblock May 18, 2021

saratvemulapalli May 18, 2021

dblock May 18, 2021 •

edited

Loading

saratvemulapalli May 18, 2021

spbjss May 18, 2021

ylwu-amzn commented May 19, 2021

ylwu-amzn commented May 19, 2021

ylwu-amzn commented May 19, 2021

saratvemulapalli commented May 19, 2021 •

edited

Loading

dblock commented May 19, 2021

saratvemulapalli commented May 19, 2021

ylwu-amzn commented May 19, 2021

dblock commented May 20, 2021

dblock commented May 20, 2021

saratvemulapalli commented May 20, 2021

spbjss commented May 20, 2021

codecov-commenter commented May 21, 2021

dblock left a comment

saratvemulapalli left a comment

saratvemulapalli May 21, 2021

saratvemulapalli May 21, 2021

dblock May 21, 2021

saratvemulapalli May 21, 2021

update the AD settings for opensearch. #47

update the AD settings for opensearch. #47

Conversation

spbjss commented May 17, 2021

Description

Issues Resolved

Check List

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

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

dblock May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn commented May 19, 2021

ylwu-amzn commented May 19, 2021

ylwu-amzn commented May 19, 2021

saratvemulapalli commented May 19, 2021 • edited Loading

dblock commented May 19, 2021

saratvemulapalli commented May 19, 2021

ylwu-amzn commented May 19, 2021

dblock commented May 20, 2021

dblock commented May 20, 2021

saratvemulapalli commented May 20, 2021

spbjss commented May 20, 2021

codecov-commenter commented May 21, 2021

Codecov Report

dblock left a comment

Choose a reason for hiding this comment

saratvemulapalli left a 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

Choose a reason for hiding this comment

dblock May 18, 2021 •

edited

Loading

saratvemulapalli commented May 19, 2021 •

edited

Loading