-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
migrate everything from java 17 to java 21 #35103
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
8701b97
to
4022950
Compare
4022950
to
ad37c14
Compare
What does this practically mean for devs? |
Excellent question! The answer is yes, and:
But that should be it. FYI, in my shell config I have
Note that airbyte-ci won't care about whether java 21 is installed or not, since it does everything in containers. |
Thanks for the reviews! I'll merge this later this evening to minimize the disruption. It'll be a force-merge: there are no significant connector changes which warrant publication. |
@postamar I think you should merge this in different PRs. The 1st PR should be for airbyte-ci. The 2nd should be the github actions. And once that's stable, we can change everything else, no? |
I see your point, I'd considered it independently, but came to the opposite conclusion. If we apply the airbyte-ci changes, which change the java runtime of the connectors, and then apply the compilation target changes later on, and then have to worry about this interim period where connectors get published with 17 bytecode but packaged in images with 21 runtimes. We could mitigate this by disabling the publishing workflow, but this still creates some risk if we're to revert one of these PRs but not the other. Hence I believe that merging (and possibly reverting) these changes atomically is actually less risky because far easier to reason about. |
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.
looks good on destinations side. destination-iceberg is failing on master too https://connectors.airbyte.com/files/generated_reports/test_summary/destination-iceberg/index.html
You can compile with a 17 target and run on 21. That’s one of the strong
guarantees of java
…---
Stephane GeneixTechnical Support
GitHub <https://github.com/airbytehq/airbyte> | Twitter
<https://twitter.com/AirbyteHQ> | LinkedIn
<https://www.linkedin.com/company/airbytehq/>
We're hiring, come work with me! <https://airbyte.io/careers> [image: 🚀]
On Fri, Feb 9, 2024 at 12:46 PM Marius Posta ***@***.***> wrote:
I see your point, I'd considered it independently, but came to the
opposite conclusion. If we apply the airbyte-ci changes, which change the
java runtime of the connectors, and then apply the compilation target
changes later on, and then have to worry about this interim period where
connectors get published with 17 bytecode but packaged in images with 21
runtimes. We could mitigate this by disabling the publishing workflow, but
this still creates some risk if we're to revert one of these PRs but not
the other. Hence I believe that merging (and possibly reverting) these
changes atomically is actually less risky because far easier to reason
about.
—
Reply to this email directly, view it on GitHub
<#35103 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BDDFPOC2H5FV3BWIOILAVALYS2DJ5AVCNFSM6AAAAABDBWXM7KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZWGU4DAMZYGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
ad37c14
to
ab9f2b3
Compare
/approve-and-merge reason="unrelated connector CI failures" |
This was fairly straightforward to do once the CDK dependencies were cleaned up.
This PR is best reviewed commit-by-commit.
While this PR triggers the connectors' CI, it does not change the connector docker images in any meaningful way. I tested that
airbyte-ci connectors --name=source-postgres build
andtest
work properly.