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

JDK 17 support #489

Merged
merged 2 commits into from
Apr 5, 2022
Merged

JDK 17 support #489

merged 2 commits into from
Apr 5, 2022

Conversation

aksingh-es
Copy link
Contributor

@aksingh-es aksingh-es commented Apr 5, 2022

Description

Support JDK 17 for AD

Added add-opens options for java.time and java.util.stream .It is no longer be possible to relax the strong encapsulation of internal elements

https://openjdk.java.net/jeps/403

Issues Resolved

[List any issues this PR will resolve]

Check List

  • 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.

@aksingh-es aksingh-es requested a review from a team April 5, 2022 01:36
@opensearch-trigger-bot opensearch-trigger-bot bot added backport 1.x infra Changes to infrastructure, testing, CI/CD, pipelines, etc. labels Apr 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2022

Codecov Report

Merging #489 (4cca200) into main (70a76b2) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #489      +/-   ##
============================================
- Coverage     78.14%   78.12%   -0.02%     
- Complexity     4156     4157       +1     
============================================
  Files           296      296              
  Lines         17652    17654       +2     
  Branches       1877     1877              
============================================
- Hits          13794    13793       -1     
- Misses         2961     2965       +4     
+ Partials        897      896       -1     
Flag Coverage Δ
plugin 78.12% <ø> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
.../java/org/opensearch/ad/AnomalyDetectorRunner.java 37.64% <0.00%> (-5.89%) ⬇️
...ava/org/opensearch/ad/model/TimeConfiguration.java 100.00% <0.00%> (ø)
...c/main/java/org/opensearch/ad/util/ParseUtils.java 77.85% <0.00%> (+0.07%) ⬆️
...opensearch/ad/indices/AnomalyDetectionIndices.java 72.12% <0.00%> (+0.18%) ⬆️
...ain/java/org/opensearch/ad/model/ModelProfile.java 70.90% <0.00%> (+1.81%) ⬆️

Copy link
Member

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -62,6 +62,8 @@ tasks.withType(JavaCompile) {
}
tasks.withType(Test) {
systemProperty "file.encoding", "UTF-8"
jvmArgs("--add-opens", "java.base/java.time=ALL-UNNAMED")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add these two lines as mock will throw exception? Xun bumpted mockito version to fix some mock issue in this PR opensearch-project/ml-commons#248. @Zhangxunmt you can add more details if that's the same issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integ tests are passing .If the mocks had issue it won't right ?

Copy link
Collaborator

@ylwu-amzn ylwu-amzn Apr 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the CI passed. I mean if don't add these two lines, the test will fail? Just provide another option if these two lines (line 65, 66) are to fix the mock issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with @aksingh-es offline, will throw these errors if remove line 65,66

ModelManagerTests > getThresholdingResult_returnExpectedToListener FAILED
  java.lang.reflect.InaccessibleObjectException: Unable to make private void java.time.Duration.readObject(java.io.ObjectInputStream) throws java.io.InvalidObjectException accessible: module java.base does not "opens java.time" to unnamed module @20fa23c1
    at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
    at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
    at java.base/java.lang.reflect.Method.checkCanSetAccessible(Method.java:199)
    at java.base/java.lang.reflect.Method.setAccessible(Method.java:193)
    at org.powermock.reflect.internal.WhiteboxImpl.doGetAllMethods(WhiteboxImpl.java:1508)
    at org.powermock.reflect.internal.WhiteboxImpl.getAllMethods(WhiteboxImpl.java:1482)
    at org.powermock.reflect.internal.WhiteboxImpl.getMethods(WhiteboxImpl.java:1750)
    at org.powermock.reflect.internal.WhiteboxImpl.getMethods(WhiteboxImpl.java:1789)
    at org.powermock.reflect.internal.WhiteboxImpl.getBestMethodCandidate(WhiteboxImpl.java:1008)
    at org.powermock.core.MockInvocation.findMethodToInvoke(MockInvocation.java:58)
    at org.powermock.core.MockInvocation.init(MockInvocation.java:35)
    at org.powermock.core.MockInvocation.<init>(MockInvocation.java:22)
    at org.powermock.core.MockGateway.doMethodCall(MockGateway.java:155)
    at org.powermock.core.MockGateway.methodCall(MockGateway.java:138)
    at org.opensearch.ad.ml.ModelManagerTests.setup(ModelManagerTests.java:190)
ModelManagerTests > getPreviewResults_returnNoAnomalies_forNoAnomalies FAILED
  java.lang.reflect.InaccessibleObjectException: Unable to make public void java.util.stream.ReferencePipeline$Head.forEach(java.util.function.Consumer) accessible: module java.base does not "opens java.util.stream" to unnamed module @20fa23c1
    at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
    at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
    at java.base/java.lang.reflect.Method.checkCanSetAccessible(Method.java:199)
    at java.base/java.lang.reflect.Method.setAccessible(Method.java:193)
    at org.powermock.reflect.internal.WhiteboxImpl.doGetAllMethods(WhiteboxImpl.java:1508)
    at org.powermock.reflect.internal.WhiteboxImpl.getAllMethods(WhiteboxImpl.java:1482)
    at org.powermock.reflect.internal.WhiteboxImpl.getMethods(WhiteboxImpl.java:1750)
    at org.powermock.reflect.internal.WhiteboxImpl.getMethods(WhiteboxImpl.java:1789)
    at org.powermock.reflect.internal.WhiteboxImpl.getBestMethodCandidate(WhiteboxImpl.java:1008)
    at org.powermock.core.MockInvocation.findMethodToInvoke(MockInvocation.java:58)
    at org.powermock.core.MockInvocation.init(MockInvocation.java:35)
    at org.powermock.core.MockInvocation.<init>(MockInvocation.java:22)
    at org.powermock.core.MockGateway.doMethodCall(MockGateway.java:155)
    at org.powermock.core.MockGateway.methodCall(MockGateway.java:138)
    at org.opensearch.ad.ml.ModelManagerTests.getPreviewResults_returnNoAnomalies_forNoAnomalies(ModelManagerTests.java:856)

Copy link
Collaborator

@ylwu-amzn ylwu-amzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aksingh-es aksingh-es merged commit e9bc74f into opensearch-project:main Apr 5, 2022
@amitgalitz amitgalitz linked an issue Apr 6, 2022 that may be closed by this pull request
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for JDK 17
4 participants