-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[#30789] Add support for Flink 1.18 #31062
Conversation
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
As I thought, seems there's a flaky test: https://github.com/apache/beam/actions/runs/8760406645/job/24045305260
I'm gonna take a look at it to see if I can identify what caused the previous run to fail. That being said, Go is not my forte. |
Thanks for the work! Yeah the flaky go SDK test is irrelevant |
sdks/typescript/package-lock.json
Outdated
@@ -1,12 +1,12 @@ | |||
{ | |||
"name": "apache-beam", | |||
"version": "2.54.0-SNAPSHOT", | |||
"version": "2.57.0-SNAPSHOT", |
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.
Though this is an unrelated change -- should this version bump be automated as part of cut-release-branch script? cc: @robertwb
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.
+1 to remove this unrelated change. We can merge this afterwards.
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'll remove it, but I didn't change it myself... won't it just get bumped again automatically?
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.
Odd, I pushed a commit but it's not showing directly in the PR's file changes. It's reflected properly in my branch and in the commit's changes, though.
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.
@thebozzcl would you please remove the unrelated change to (possibly) different PR? This can be merged then (after checks pass).
sdks/typescript/package-lock.json
Outdated
@@ -1,12 +1,12 @@ | |||
{ | |||
"name": "apache-beam", | |||
"version": "2.54.0-SNAPSHOT", | |||
"version": "2.57.0-SNAPSHOT", |
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.
+1 to remove this unrelated change. We can merge this afterwards.
@@ -101,5 +101,5 @@ jobs: | |||
with: | |||
gradle-command: :sdks:java:testing:tpcds:run | |||
arguments: | | |||
-Ptpcds.runner=:runners:flink:1.17 \ | |||
-Ptpcds.runner=:runners:flink:1.18 \ |
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.
Sorry for not putting comments at once, but just realize that these github action changes will apply to both the master branch and the ongoing release process, but the current pending release-2.56.0 branch do not have :runners:flink:1.18
project which will causing issue
To avoid issue we can leave .github/ content as is
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.
We might merge this after the release of 2.56.0?
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'm in no rush to get this merged, so I'd be okay with that.
sdks/typescript/package-lock.json
Outdated
@@ -6,7 +6,7 @@ | |||
"packages": { | |||
"": { | |||
"name": "apache-beam", | |||
"version": "2.54.0-SNAPSHOT", | |||
"version": "2.57.0-SNAPSHOT", |
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.
Strange, there is still this change, which might cause the failure of typescript check?
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 think I finally understood what happened here: there's two lines with the package version, and I failed to notice the second one.
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.
thanks for updating the PR, @thebozzcl. One last thing, can you please squash the commits? I'll merge this once the release of 2.56.0 is out and once the checks pass.
Assigning reviewers. If you would like to opt out of this review, comment R: @jrmccluskey for label go. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
R: @je-ik |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Looks like this is all green and ready to merge. Can you please squash the commits to single commit? Thanks. 👍 |
Actually committer can "squash and merge" using GitHub website. No further action needed for PR author. |
@Abacn Cool, I was not aware of that 👍 |
The task Below is the latest error from the log:
|
Hmm. I'm gonna try looking into it, but I have limited connectivity at the moment and might not be able to do much until later this week. What's the preferred way to deal with this? Do we want to roll back? |
This reverts commit 45fe4f9.
I can confirm the failure locally, we should revert this until we can make the check pass. |
Correction. The test locally fails even before this commit (e.g. tested |
I took a "quick" look (it took a while to dl all the dependencies in a connection that's not much better than dial-up). I'm kind of baffled by this issue - I checked the We should probably create a separate ticket for this. |
@je-ik PostRelease test is different from other tests. It downloads the latest Nightly Snapshot and run some validation pipelines. It does not build beam on the current branch, and that is why you see "The test locally fails even before this commit". |
@je-ik, @Abacn: Given the task |
Looks like someone have reported this issue on Flink 1.18 before. |
I agree revert at the change moment
This means current master branch is failing Flink runner validation for both Flink 1.18 and under 1.18, possibly caused by this PR. If we revert the change and wait for another nightly snapshot built, and the PostRelease test is back green, we can confirm |
if revert, there is another PR need to be reverted at the same time : #31217 This one included a typo fix which shouldn't be reverted : https://github.com/apache/beam/pull/31217/files#diff-07d5f4d101410e8cf42b7241fe72074d3c8003866e2b077f51388e1283ebe442R337 |
Checking this PR again, it does not include non-trivial change. I doubt that revert it will make the flinkQuickstart back green again though |
This reverts commit 557b5ba.
Just for posterity and everyone else getting notifications. As release manager I learned just now that we need a tickets such as this whenever we add a new version: https://issues.apache.org/jira/browse/INFRA-25880 These can take a few days to get resolved, so it is good to do it right away as part of adding the new version support. |
A proposal to use single dockerhub repository for Flink job server container: #31631 |
This PR tries to solve #30789 by adding a runner for Flink 1.18. So far I haven't identified any changes that need to be done beyond the version bump.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.