-
Notifications
You must be signed in to change notification settings - Fork 138
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
fix commons-lang3 package version to fix jarhell issue #1288
Conversation
Signed-off-by: Sicheng Song <[email protected]>
Signed-off-by: Sicheng Song <[email protected]>
@@ -189,7 +189,7 @@ integTest { | |||
} | |||
|
|||
testClusters.integTest { | |||
testDistribution = "ARCHIVE" | |||
testDistribution = "INTEG_TEST" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change to INTEG_TEST
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not we will get this error in integ_test:
| Caused by: java.lang.IllegalStateException: jar hell!
| class: com.amazonaws.encryptionsdk.CryptoMaterialsManager
| jar1: /local/home/dev/ml-commons/plugin/build/testclusters/integTest-0/distro/3.0.0-ARCHIVE/lib/aws-encryption-sdk-java-2.4.0.jar
| jar2: /local/home/dev/ml-commons/plugin/build/testclusters/integTest-0/distro/3.0.0-ARCHIVE/plugins/.installing-2105471977921602206/aws-encryption-sdk-java-2.4.0.jar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm.. I suspect we are going to see problems.
INTEG_TEST
does not pull in modules which might work for us for tests but when we have to run it with the distribution(release) it will have modules installed and we will end up with JarHell.
Could you manually try installing the plugin with OpenSearch 2.x locally assembled with this change?
Can you verify we dont hit the problem ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. This fix is introduced by @HenryL27 so I think we need to involve him in the loop. If so do you have any idea on fixing this jarhell issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried in my local, with/without changing this, using distribution won't have jar hell issue.
Codecov Report
@@ Coverage Diff @@
## main #1288 +/- ##
============================================
- Coverage 82.31% 78.89% -3.43%
- Complexity 1920 2141 +221
============================================
Files 149 168 +19
Lines 7508 8740 +1232
Branches 750 877 +127
============================================
+ Hits 6180 6895 +715
- Misses 988 1447 +459
- Partials 340 398 +58
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Close this due to the core PR has been reverted. |
Description
Follows opensearch-project/security-analytics#535 to fix jarhell issue
Check List
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.