Code Refactoring for CommonName by kaituo · Pull Request #867 · opensearch-project/anomaly-detection

kaituo · 2023-04-15T04:47:22Z

Description

In this pull request, I have refactored the code related to shared names in both AD and forecasting modules to CommonNames. Additionally, the previously used CommonName has been renamed to ADCommonName. For the Forecasting module, I have introduced new names in ForecastCommonNames.

Testing done:

gradle build

Issues Resolved

#866

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.

In this pull request, I have refactored the code related to shared names in both AD and forecasting modules to CommonNames. Additionally, the previously used CommonName has been renamed to ADCommonName. For the Forecasting module, I have introduced new names in ForecastCommonNames. Testing done: * gradle build Signed-off-by: Kaituo Li <kaituo@amazon.com>

codecov-commenter · 2023-04-15T05:19:13Z

Codecov Report

❌ Patch coverage is 81.34715% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.35%. Comparing base (1edc2ef) to head (44622ba).
⚠️ Report is 2 commits behind head on feature-forecast.

Files with missing lines	Patch %	Lines
.../java/org/opensearch/ad/model/DetectorProfile.java	71.42%	6 Missing ⚠️
.../main/java/org/opensearch/ad/ml/CheckpointDao.java	66.66%	4 Missing ⚠️
.../handler/AbstractAnomalyDetectorActionHandler.java	42.85%	4 Missing ⚠️
...in/java/org/opensearch/ad/model/EntityProfile.java	50.00%	3 Missing ⚠️
...rg/opensearch/ad/AnomalyDetectorProfileRunner.java	66.66%	1 Missing and 1 partial ⚠️
.../ad/rest/handler/AnomalyDetectorActionHandler.java	0.00%	2 Missing ⚠️
...arch/ad/transport/EntityResultTransportAction.java	0.00%	1 Missing and 1 partial ⚠️
...c/main/java/org/opensearch/ad/util/ClientUtil.java	0.00%	2 Missing ⚠️
...c/main/java/org/opensearch/ad/util/ParseUtils.java	50.00%	1 Missing and 1 partial ⚠️
.../java/org/opensearch/ad/constant/ADCommonName.java	0.00%	1 Missing ⚠️
... and 8 more

Additional details and impacted files

@@                  Coverage Diff                   @@
##             feature-forecast     #867      +/-   ##
======================================================
- Coverage               79.54%   79.35%   -0.19%     
+ Complexity               4312     4296      -16     
======================================================
  Files                     306      308       +2     
  Lines                   18095    18099       +4     
  Branches                 1889     1889              
======================================================
- Hits                    14394    14363      -31     
- Misses                   2792     2829      +37     
+ Partials                  909      907       -2

Flag	Coverage Δ
plugin	`93.75% <81.34%> (+14.20%)`	⬆️

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

Files with missing lines	Coverage Δ
...va/org/opensearch/ad/AnomalyDetectorJobRunner.java	`80.50% <100.00%> (ø)`
.../java/org/opensearch/ad/AnomalyDetectorPlugin.java	`96.68% <100.00%> (ø)`
...in/java/org/opensearch/ad/EntityProfileRunner.java	`75.40% <100.00%> (ø)`
.../main/java/org/opensearch/ad/NodeStateManager.java	`76.47% <100.00%> (+1.96%)`	⬆️
src/main/java/org/opensearch/ad/ProfileUtil.java	`94.44% <100.00%> (ø)`
...java/org/opensearch/ad/cluster/ADDataMigrator.java	`79.08% <100.00%> (ø)`
.../java/org/opensearch/ad/cluster/ADVersionUtil.java	`93.33% <ø> (ø)`
...main/java/org/opensearch/ad/cluster/DailyCron.java	`100.00% <100.00%> (ø)`
.../main/java/org/opensearch/ad/cluster/HashRing.java	`81.78% <ø> (ø)`
...ter/diskcleanup/ModelCheckpointIndexRetention.java	`82.92% <100.00%> (ø)`
... and 67 more

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sudiptoguha

Looks good - perhaps there are followups in the name change?

sudiptoguha · 2023-04-17T17:48:46Z

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

            cacheProvider,
            checkpoint,
-            CommonName.CHECKPOINT_INDEX_NAME,
+            ADCommonName.CHECKPOINT_INDEX_NAME,


small nit: AnomalyDetectorCommonName ?? The line below is AnomalyDetectorSettings so ... but yes an acronym would make the lines shorter and readable.

yes, trying to use acronym to make the lines shorter and readable.

sudiptoguha · 2023-04-17T17:49:01Z

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

            AnomalyDetectorSettings.QUEUE_MAINTENANCE,
            checkpoint,
-            CommonName.CHECKPOINT_INDEX_NAME,
+            ADCommonName.CHECKPOINT_INDEX_NAME,


Same as previous.

yes, trying to use acronym to make the lines shorter and readable.

sudiptoguha · 2023-04-17T17:53:21Z

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

            .put(
                StatNames.ANOMALY_DETECTORS_INDEX_STATUS.getName(),
-                new ADStat<>(true, new IndexStatusSupplier(indexUtils, AnomalyDetector.ANOMALY_DETECTORS_INDEX))
+                new ADStat<>(true, new IndexStatusSupplier(indexUtils, CommonName.CONFIG_INDEX))


The "ADStat" should also get updated to "Stat/PluginStat"? But that can/should be a different PR.

yes, will do that in a different PR.

sudiptoguha · 2023-04-17T20:25:09Z

src/main/java/org/opensearch/ad/constant/ADCommonName.java

    // ======================================
    // Index name
    // ======================================
    // index name for anomaly checkpoint of each model. One model one document.
    public static final String CHECKPOINT_INDEX_NAME = ".opendistro-anomaly-checkpoints";
    // index name for anomaly detection state. Will store AD task in this index as well.
    public static final String DETECTION_STATE_INDEX = ".opendistro-anomaly-detection-state";
-    // TODO: move other index name here


Any plans to change the "opendistro-anomaly" moniker? Isn't "OpenSearch" more appropriate? Can be a different PR though. OpenDistro may indicate that this is serving dated models (which is not the case AFAIK).

To keep it backward-compatible, we will use opendistro as part of index name. For new indices (e.g., forecasting indices), we are gonna use opensearch.

In this pull request, I have refactored the code related to shared names in both AD and forecasting modules to CommonNames. Additionally, the previously used CommonName has been renamed to ADCommonName. For the Forecasting module, I have introduced new names in ForecastCommonNames. Testing done: * gradle build Signed-off-by: Kaituo Li <kaituo@amazon.com>

@parameters

* Add 2.7 release notes (#871) Signed-off-by: Jackie Han <jkhanjob@gmail.com> Signed-off-by: Kaituo Li <kaituo@amazon.com> * Various fixes (#886) * Various fixes This pull request addresses several issues related to the compiler and tests in the current main branch. The first major change involves replacing ImmutableOpenMap with java.util.Map in the core. This modification is implemented in the following pull requests: opensearch-project/OpenSearch#7165 opensearch-project/OpenSearch#7301 To accommodate this change, the codebase of the AD (Anomaly Detection) module has been refactored to utilize JDK maps. As a consequence of this change, passing null to the custom parameters of ClusterState is no longer permissible, as it leads to a NullPointerException. The error stack trace is as follows: java.lang.NullPointerException: Cannot invoke "Object.getClass()" because "m" is null at __randomizedtesting.SeedInfo.seed([60CDDB34427ACD0C:6E72DB4ED18E018D]:0) at java.base/java.util.Collections.unmodifiableMap(Collections.java:1476) at org.opensearch.cluster.ClusterState.<init>(ClusterState.java:219) at org.opensearch.ad.transport.DeleteAnomalyDetectorTests.createClusterState(DeleteAnomalyDetectorTests.java:216) at org.opensearch.ad.transport.DeleteAnomalyDetectorTests.testDeleteADTransportAction_LatestDetectorLevelTask(DeleteAnomalyDetectorTests.java:160) To address this issue, we have replaced the usage of null with new HashMap<>(). The second change in this pull request aligns with the modifications introduced in PR #878. The third issue is related to the incompatibility between tests that utilize the @parameters annotation and those that do not, as explained in https://tinyurl.com/2y265s2w. Specifically, the SearchFeatureDaoTests class runes tests with the @parameters annotation, whereas SearchFeatureDao tests do not. Testing done: 1. gradle build. Signed-off-by: Kaituo Li <kaituo@amazon.com> * Code Refactoring for CommonName (#867) In this pull request, I have refactored the code related to shared names in both AD and forecasting modules to CommonNames. Additionally, the previously used CommonName has been renamed to ADCommonName. For the Forecasting module, I have introduced new names in ForecastCommonNames. Testing done: * gradle build Signed-off-by: Kaituo Li <kaituo@amazon.com> --------- Signed-off-by: Jackie Han <jkhanjob@gmail.com> Signed-off-by: Kaituo Li <kaituo@amazon.com> Co-authored-by: Jackie Han <hnyng@amazon.com>

sudiptoguha

Same c comment as 901, looks good.

kaituo requested review from a team and jackiehanyang April 15, 2023 04:47

sudiptoguha approved these changes Apr 17, 2023

View reviewed changes

jackiehanyang approved these changes Apr 28, 2023

View reviewed changes

kaituo merged commit 2b9a25f into opensearch-project:feature-forecast Apr 28, 2023

kaituo mentioned this pull request May 12, 2023

cherrypick Code Refactoring for CommonName to main #901

Merged

sudiptoguha reviewed May 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Refactoring for CommonName#867

Code Refactoring for CommonName#867
kaituo merged 1 commit intoopensearch-project:feature-forecastfrom
kaituo:commonName

kaituo commented Apr 15, 2023

Uh oh!

codecov-commenter commented Apr 15, 2023 •

edited by codecov bot

Loading

Uh oh!

sudiptoguha left a comment

Uh oh!

sudiptoguha Apr 17, 2023

Uh oh!

kaituo Apr 24, 2023

Uh oh!

sudiptoguha Apr 17, 2023

Uh oh!

kaituo Apr 24, 2023

Uh oh!

sudiptoguha Apr 17, 2023

Uh oh!

kaituo Apr 24, 2023

Uh oh!

sudiptoguha Apr 17, 2023

Uh oh!

kaituo Apr 24, 2023

Uh oh!

sudiptoguha left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kaituo commented Apr 15, 2023

Description

Issues Resolved

Uh oh!

codecov-commenter commented Apr 15, 2023 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sudiptoguha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sudiptoguha left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Apr 15, 2023 •

edited by codecov bot

Loading