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

Forecast rebase #883

Merged
merged 3 commits into from
May 11, 2023
Merged

Conversation

kaituo
Copy link
Collaborator

@kaituo kaituo commented Apr 28, 2023

Description

rebase commit #871 and #886 from main on top of forecast feature branch.

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.

@kaituo kaituo requested review from sudiptoguha, ohltyler and a team April 28, 2023 17:16
jackiehanyang
jackiehanyang previously approved these changes Apr 28, 2023
sudiptoguha
sudiptoguha previously approved these changes Apr 28, 2023
@kaituo kaituo force-pushed the forecast-rebase branch 2 times, most recently from 7edb33b to 78fe8bc Compare May 9, 2023 21:42
jackiehanyang and others added 3 commits May 9, 2023 15:14
Signed-off-by: Jackie Han <[email protected]>
Signed-off-by: Kaituo Li <[email protected]>
* 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 opensearch-project#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]>
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 kaituo force-pushed the forecast-rebase branch from 78fe8bc to ac16fa5 Compare May 9, 2023 22:14
@kaituo
Copy link
Collaborator Author

kaituo commented May 10, 2023

bwc tests and security tests failed due to same reason mentioned in #886

Copy link

@sudiptoguha sudiptoguha 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.

@kaituo kaituo merged commit 550f44f into opensearch-project:feature-forecast May 11, 2023
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.

4 participants