Upgrade build to be compatible with JDK17

[aaneja]

Co-authored-by: Zac Blanco zac@ibm.com
Co-authored-by: Jalpreet Singh Nanda (:imjalpreet) jalpreetnanda@gmail.com

Description

Upgrade build to be compatible with JDK17 while still retaining support for JDK 8

Motivation and Context

https://github.com/prestodb/rfcs/blob/main/RFC-0010-jdk-upgrade.md

Impact

None

Test Plan

CI

Contributor checklist

• Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.

Release Notes

== RELEASE NOTES ==

General Changes
* Add support to build Presto with JDK 17

[rschlussel]

why was this removed?

[ZacBlanco]

IIRC This was causing issues with the matrix builds because it was cancelling already in-progress actions. We removed it so we could get both versions testing. That action is actually deprecated and we should migrate off of it: https://github.com/n1hility/cancel-previous-runs?tab=readme-ov-file#note-this-action-was-created-before-concurrency-groups-in-most-cases-you-want-to-define-a-cancel-in-progress-group-instead-of-using-this-action

Also, the CI matrix is actually something we wanted to bring up at the TSC because we weren't sure how folks might feel about resource requirements and turnaround time on PRs when we almost double the number of actions per PR by running tests on both versions.

[rschlussel]

Thanks for this! All the main code looks good. 34 tests (not-matrixed versions of the other tests, so probably shouldn't be running at all?) seem to be stuck.

[ZacBlanco]

I think the 34 "stuck" tests are a product of the repository configuration's existing branch protection rules. Since they are marked as required in the repository configuration, it shows them in the list here. The original checks will of course not run because we modified the GitHub actions CI to use a matrix so the names of the checks have changed. It seems the branch protection rules only look for the names of the checks

These are the two options on how to proceed around the CI:

1. Update branch protection rules to match the java 8 and 17 versions of required checks based on this PR after we merge it (overriding current branch protection rules).
2. Revert the changes to the CI in this PR in order to get status checks all passing. Re-introduce them at a later time after discussion of which java 17 checks are necessary to run on every PR and update branch protection rules accordingly

[rschlussel]

either is okay with me. My instinct is to merge as is, and if we find the 2x checks are too onerous we can choose which ones to remove. Do you have a preference?

[ZacBlanco]

I would prefer to have the 17 checks run and then reduce from there if necessary. If we merge this will you be able to update branch protection rules?

[rschlussel]

yeah, I can update. It's a very manual process with a weird UI, so i may miss some. if you notice anything wrong, just tag me to fix it.

[czentgr]

Why do we want to force the build for the other java version if one of them fails?

[ZacBlanco]

short offline discussion: we disabled fail-fast initially during our testing so that we could view all the errors for both 8 & 17 builds. This shouldn't be required any more though. Will create a follow-up PR to remove fail-fast (the default is true)

[rschlussel]

required tests should all be updated now. #24689 tests look as expected.

[ZacBlanco]

Great, thank you @rschlussel !

[rschlussel]

One more thing: I noticed the arrow flight tests don't use a matrix (e.g. https://github.com/prestodb/presto/actions/runs/13725273780/job/38390098449?pr=24689)

[ZacBlanco]

I will add the matrix to the arrow-flight tests in this follow up PR for the actions #24690