Pinging @elastic/es-core-infra (:Core/Infra/Packaging)

Thanks for the PR @james-crowley. As explained in #55125 (comment), we don't plan to merge this change at the moment, so I hope you don't mind that I close it.

@rjernst In light of recent discussions I think this is worth reconsideration. One thing we didn't really account for in sticking with the existing Artifactory solution is network stability/availability. Our internal Artifactory is a single point of failure, and isn't fronted by any kind of CDN or otherwise regionally distributed. This both makes external contributors susceptible to interruption by outages of our own infrastructure as well as less than great experience for those no located in the US where this infrastructure lives.

With that in mind I propose we move forward with merging this in, take the optimistic view regarding the stability of the v3 API for AdoptOpenJDK and then revisit when a) we have an HA solution for Artifactory or b) the JVM catalog becomes a viable solution.

@elasticmachine update branch

@elasticmachine test this please

@james-crowley do you mind taking a look at these test failures. I believe we'll need to update that integration test and possibly the associated fixture to account for the new artifact url pattern.

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-1/21784/testReport/

@mark-vieira Let me take a look now!

@mark-vieira Seems like all those tests fail because of the pattern. I am adding a fix in and testing it locally now.

I think the issue is on line 126, I need to use jdk.getBaseVersion() not jdk.getMajor(). This should allow for versions like 12.0.2 to get downloaded fine.

The other issue is with your test for the legacy pattern.

What version of Java is 1+99 suppose to be targeting? If you can point me in the right direction I can see if I need to update the case for legacy patterns. But from just 1+99 I am not sure what version I need to check on. Mind pointing me in the right direction?

The "old version" is referring to Oracle changing their url, which IIRC was with java 12.0.1, to include a hash. The test was modified to test both the old and new patterns. When adoptopenjdk support was added, the test was reused, and this test was supported there, but in practice it wasn't actually used since JdkDownloadPlugin does not have any change of the url pattern in handling adoptopenjdk. I think we can reorganize this test to make the old jdk test specific to openjdk, not adoptopenjdk. I'll open a quick PR for this.

@james-crowley FYI #55361

@rjernst Thanks for the PR. My changes looked like they worked now.

> Task :build-tools:integTest

org.elasticsearch.gradle.AdoptOpenJdkDownloadPluginIT > testDarwinExtractionOldVersion FAILED
    org.gradle.testkit.runner.UnexpectedBuildFailure at [F7AB7FF3DC54F816:E42CADF3287BD59]:0

org.elasticsearch.gradle.AdoptOpenJdkDownloadPluginIT > testWindowsExtractionOldVersion FAILED
    org.gradle.testkit.runner.UnexpectedBuildFailure at [F7AB7FF3DC54F816:D912418897DAA3F8]:0

org.elasticsearch.gradle.AdoptOpenJdkDownloadPluginIT > testLinuxExtractionOldVersion FAILED
    org.gradle.testkit.runner.UnexpectedBuildFailure at [F7AB7FF3DC54F816:74B4B3B1F1243DC9]:0

151 tests completed, 3 failed, 1 skipped

> Task :build-tools:integTest FAILED

The only test failing now were the old JDK version, which your PR just removed.

Let me clean up my changes and pull in your PR after it gets merged, and I'll update this PR.

@jasontedor @rjernst or @mark-vieira Might kicking off the tests/CI? I updated the PR to pull in the new code, updated the tests and a fix for Java versions like 12.0.2.

@elasticmachine test this please

@jasontedor @rjernst @mark-vieira No rush but the two failed CI jobs, default-distro and bwc, do not look related to what I changed. Anything I can do to help debug them? Are they blockers for merging this PR?

@elasticmachine update branch

@elasticmachine test this please

@james-crowley That can happen when changes in the master branch needed for BWC purposes are not merged into the PR branch. We hold a fairly strict line on "all checks are green" before we merge PRs, even if they appear unrelated.

Looks like we're green. I'll go ahead and merge and backport this. Thanks @james-crowley!

Thanks a lot @james-crowley.

@jasontedor @rjernst @mark-vieira Thanks for all the help with getting this merged!