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

fix missing commons-lang3 #363

Merged
merged 2 commits into from
Jan 14, 2022

Conversation

ylwu-amzn
Copy link
Collaborator

@ylwu-amzn ylwu-amzn commented Jan 14, 2022

Signed-off-by: Yaliang Wu [email protected]

Description

Fix missing commons lang3 when start OpenSearch cluster by changing compileOnly to compile.
We should also use latest job-scheduler zip to pass build as the old zip has duplicate commons-lang3 which will cause jar hell

| class: org.apache.commons.lang3.AnnotationUtils$1
| jar1: /local/home/ylwu/code/os/anomaly-detection-2/build/testclusters/integTest-0/distro/1.2.4-ARCHIVE/plugins/opensearch-job-scheduler/opensearch-job-scheduler-spi-1.2.4.0-SNAPSHOT.jar
| jar2: /local/home/ylwu/code/os/anomaly-detection-2/build/testclusters/integTest-0/distro/1.2.4-ARCHIVE/plugins/.installing-12824997717448853161/commons-lang3-3.12.0.jar
|       at org.opensearch.bootstrap.JarHell.checkClass(JarHell.java:317)
|       at org.opensearch.bootstrap.JarHell.checkJarHell(JarHell.java:212)
|       at org.opensearch.plugins.PluginsService.checkBundleJarHell(PluginsService.java:662)
|       ... 11 more

Why AD passed CI:

  1. Job scheduler team upgraded cron-utils in this PR Bumps cron-utils version to 9.1.6 job-scheduler#118, which will not include commons-lang3
  2. Then AD team added commons-lang3 dependency explicitly. But we used old job-scheduler 1.2.4 zip which still has commons-lang3 packed. So it was ok to use compileOnly in AD and CI can pass.
  3. In the release tarball/dockerImage, the job-scheduler is built from latest code which doesn't have commons-lang3 packed any more, then AD can't find commons-lang3.

We plan to pull job scheduler from maven or build locally with latest code or build JobScheduler plugin zip from source to avoid such issue.

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.

Signed-off-by: Yaliang Wu <[email protected]>
Zhangxunmt
Zhangxunmt previously approved these changes Jan 14, 2022
amitgalitz
amitgalitz previously approved these changes Jan 14, 2022
Copy link
Member

@amitgalitz amitgalitz left a comment

Choose a reason for hiding this comment

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

LGTM, we should wait until CI passes then merge.

@Zhangxunmt
Copy link
Contributor

https://stackoverflow.com/questions/56878268/build-gradle-compile-deprecated-warning. One note is that we will need to deprecate "compile" at some point.

@ylwu-amzn ylwu-amzn dismissed stale reviews from amitgalitz and Zhangxunmt via 9241880 January 14, 2022 02:52
@ylwu-amzn ylwu-amzn merged commit 32cb1d1 into opensearch-project:1.2 Jan 14, 2022
@ohltyler
Copy link
Member

@ylwu-amzn does this need to be front-ported to main too?

@ylwu-amzn
Copy link
Collaborator Author

@ylwu-amzn does this need to be front-ported to main too?

Yes, send out PR #364

@dblock
Copy link
Member

dblock commented Jan 14, 2022

I would expect some test to fail in anomaly-detection CI if it's going to cause OpenSearch not to start. Are we missing something @ohltyler @ylwu-amzn?

@amitgalitz
Copy link
Member

I would expect some test to fail in anomaly-detection CI if it's going to cause OpenSearch not to start. Are we missing something @ohltyler @ylwu-amzn?

I might be wrong on this but initially when it first failed we couldn't compile at all. After that we added the dependency as "compileOnly" meaning it didn't cause any build issues but the most up to version dependency wasn't bundled with the JAR so it failed in runtime for opensearch. It passed following tests in our case because we didn't have the most up to date job-scheduler-1.2.4 in our repo that took out the dependency recently so we weren't testing with the exact same version as was in opensearch 1.2.4 official build. This is another indicator in my opinion that we really need to be pulling job-scheduler from maven. @ylwu-amzn @ohltyler does this make sense or should we be adding additional testing for after the artificat is built.

@ylwu-amzn
Copy link
Collaborator Author

ylwu-amzn commented Jan 14, 2022

I would expect some test to fail in anomaly-detection CI if it's going to cause OpenSearch not to start. Are we missing something @ohltyler @ylwu-amzn?

I might be wrong on this but initially when it first failed we couldn't compile at all. After that we added the dependency as "compileOnly" meaning it didn't cause any build issues but the most up to version dependency wasn't bundled with the JAR so it failed in runtime for opensearch. It passed following tests in our case because we didn't have the most up to date job-scheduler-1.2.4 in our repo that took out the dependency recently so we weren't testing with the exact same version as was in opensearch 1.2.4 official build. This is another indicator in my opinion that we really need to be pulling job-scheduler from maven. @ylwu-amzn @ohltyler does this make sense or should we be adding additional testing for after the artificat is built.

@dblock Amit's explanation is correct, you can also check the description part of this PR.

We built job-scheduler plugin zip from source code before, but this logic removed and just depends static job-scheduler zip now after this PR #286

To make sure we always use the latest job-scheduler artifact in AD CI, we can either pull job-scheduler plugin zip from somewhere like Maven but job-scheduler team must make sure they always publish the latest plugin zip before AD CI. The other easier way to avoid such potential circular dependency(OpenSearch build workflow needs both AD and JobScheduler ready to publish JobScheduler to Maven, and AD needs JobScheduler ready to pass CI) is change back to old logic to build JobScheduler plugin zip from source code in AD CI. What do you think/prefer, @dblock, @amitgalitz ?

@dblock
Copy link
Member

dblock commented Jan 14, 2022

Thanks, I understand. We will fix the circular dependency in opensearch-project/opensearch-build#1463, so I think there's nothing else to do here.

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.

5 participants