Code Refactoring for CommonName #867
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <[email protected]>
Codecov Report
📣 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-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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
sudiptoguha
approved these changes
Apr 17, 2023
| @@ -454,7 +455,7 @@ public PooledObject<LinkedBuffer> wrap(LinkedBuffer obj) { | |||
| CheckPointMaintainRequestAdapter adapter = new CheckPointMaintainRequestAdapter( | |||
| cacheProvider, | |||
| checkpoint, | |||
| CommonName.CHECKPOINT_INDEX_NAME, | |||
| ADCommonName.CHECKPOINT_INDEX_NAME, | |||
There was a problem hiding this comment.
small nit: AnomalyDetectorCommonName ?? The line below is AnomalyDetectorSettings so ... but yes an acronym would make the lines shorter and readable.
There was a problem hiding this comment.
yes, trying to use acronym to make the lines shorter and readable.
| @@ -477,7 +478,7 @@ public PooledObject<LinkedBuffer> wrap(LinkedBuffer obj) { | |||
| AnomalyDetectorSettings.MAINTENANCE_FREQ_CONSTANT, | |||
| AnomalyDetectorSettings.QUEUE_MAINTENANCE, | |||
| checkpoint, | |||
| CommonName.CHECKPOINT_INDEX_NAME, | |||
| ADCommonName.CHECKPOINT_INDEX_NAME, | |||
There was a problem hiding this comment.
yes, trying to use acronym to make the lines shorter and readable.
| @@ -625,23 +626,23 @@ public PooledObject<LinkedBuffer> wrap(LinkedBuffer obj) { | |||
| ) | |||
| .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)) | |||
There was a problem hiding this comment.
The "ADStat" should also get updated to "Stat/PluginStat"? But that can/should be a different PR.
There was a problem hiding this comment.
yes, will do that in a different PR.
| // ====================================== | ||
| // 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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
jackiehanyang
approved these changes
Apr 28, 2023
kaituo
added a commit
to kaituo/anomaly-detection-1
that referenced
this pull request
Apr 28, 2023
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 <[email protected]>
kaituo
added a commit
to kaituo/anomaly-detection-1
that referenced
this pull request
Apr 28, 2023
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 <[email protected]>
kaituo
added a commit
to kaituo/anomaly-detection-1
that referenced
this pull request
May 9, 2023
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 <[email protected]>
kaituo
added a commit
to kaituo/anomaly-detection-1
that referenced
this pull request
May 9, 2023
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 <[email protected]>
kaituo
added a commit
to kaituo/anomaly-detection-1
that referenced
this pull request
May 9, 2023
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 <[email protected]>
kaituo
added a commit
that referenced
this pull request
May 11, 2023
* Add 2.7 release notes (#871) Signed-off-by: Jackie Han <[email protected]> Signed-off-by: Kaituo Li <[email protected]> * 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 <[email protected]> * 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 <[email protected]> --------- Signed-off-by: Jackie Han <[email protected]> Signed-off-by: Kaituo Li <[email protected]> Co-authored-by: Jackie Han <[email protected]>
sudiptoguha
reviewed
May 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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.