Setup Java Flink runner PreCommit #27721
Conversation
* Split Flink 1.13-1.15 tests from main Java PreCommit
|
Example workflow run: https://github.com/Abacn/beam/actions/runs/5685099288/job/15409247648 |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @damccorm for label build. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
build.gradle.kts
Outdated
| dependsOn(":runners:flink:${flinkVersions[0]}:build") | ||
| dependsOn(":runners:flink:${flinkVersions[0]}:job-server:build") | ||
| dependsOn(":runners:flink:${flinkVersions[flinkVersions.size - 1]}:build") | ||
| dependsOn(":runners:flink:${flinkVersions[flinkVersions.size - 1]}:job-server:build") |
There was a problem hiding this comment.
How often are we aware of catching a java issue with flink specific tests? I'd probably vote we just move all versions into the separate suite where it can run only on flink changes. In general, I'm a big fan of smaller more targeted suites to reduce flakiness.
If the flink tests would've caught a real issue, we'll still get that when the action does its normal scheduled run.
There was a problem hiding this comment.
How often are we aware of catching a java issue with flink specific tests?
I do not remember a specific example, but I imagine this could happen in general.
We may still need at least one exercised in the main test suite, for the purpose of code coverage stat.
Similar practice is done in Spark runner precommit. In main test suite it runs with Spark 3.2; there is an Spark3_Version Precommit run additonal versions:
beam/runners/spark/3/build.gradle
Line 35 in a4f2934
Will make the main precommit suite run only one flink version, and rename the test
There was a problem hiding this comment.
Ok; my vote is to split it out entirely, but I can live with just one suite
We may still need at least one exercised in the main test suite, for the purpose of code coverage stat.
I don't think this is a good reason to make decisions like this. Code coverage is just a number that can be a useful indicator of places we have gaps, if its keeping us from making optimal decisions about our infrastructure we should ignore it.
| with: | ||
| cache-read-only: false | ||
| - name: run Java Flink PreCommit script | ||
| run: ./gradlew :flinkPreCommit |
There was a problem hiding this comment.
We should use the gradle action to run the task in order to add the required params - https://github.com/apache/beam/blob/master/.github/actions/gradle-command-self-hosted-action/action.yml#L48-L49
There was a problem hiding this comment.
Thanks. I copy pasted (Java_Example_Dataflow) workflow. Will do
There was a problem hiding this comment.
now using gradle-command-self-hosted-action
| issue_comment: | ||
| types: [created] | ||
| schedule: | ||
| - cron: '20 */6 * * *' |
There was a problem hiding this comment.
What is the intention to run the workflow exactly at the 20th minute?
There was a problem hiding this comment.
manually add some randomness to distribute the job, then not all cron job start at 0th minute
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| name: Java Flink Runner PreCommit |
There was a problem hiding this comment.
Can we put PreCommit at the beginning of the workflow name? The intention is to keep and list all PreCommit workflow files together
There was a problem hiding this comment.
Sounds good! will change
There was a problem hiding this comment.
moved PreCommit to the first word
| name: beam_PreCommit_Java_Flink_Versions (Run Java_Flink_Versions PreCommit) | ||
| steps: | ||
| - name: Git checkout | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
This step should use ref as per the README file - https://github.com/apache/beam/blob/master/.github/ACTIONS.md?plain=1#L65-L68. However, it should probably be different for the issue_comment event due to the issue (the fix is on my plate)
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| beam_PreCommit_Java_Flink_Versions: |
There was a problem hiding this comment.
We also need to add the timeout-minutes: limit (see the example). The default GH limit is 360, but the default limit that is used for jenkins jobs is 120 - https://github.com/apache/beam/blob/master/.test-infra/jenkins/PrecommitJobBuilder.groovy#L39. We should use this to keep track of whether we have a performance regression or not.
|
Workflow run successfully on fork: https://github.com/Abacn/beam/actions/runs/5694030121/job/15434371968 PTAL |
|
Thanks @damccorm for review. Hi @andreydevyatkin would you mind taking another look about the changes made, thanks! |
|
There are fixes on PreCommits yml ongoing (dispatch, trigger, etc), will integrate the fixed in this workflow also. |
| if: | | ||
| github.event_name == 'push' || | ||
| github.event_name == 'pull_request_target' || | ||
| github.event_name == 'schedule' || |
There was a problem hiding this comment.
Can you please add a condition for the workflow_dispatch event?
There was a problem hiding this comment.
I see current workflows has the same (just a workflow_dispatch). What should be the pattern here?
There was a problem hiding this comment.
The new changes were recently merged. The following condition should be added:
github.event_name == 'workflow_dispatch' ||
| if: | | ||
| github.event_name == 'push' || | ||
| github.event_name == 'pull_request_target' || | ||
| github.event_name == 'schedule' || |
There was a problem hiding this comment.
The new changes were recently merged. The following condition should be added:
github.event_name == 'workflow_dispatch' ||
| - name: Git checkout | ||
| uses: actions/checkout@v3 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| - name: Rerun on comment | ||
| if: github.event.comment.body == 'Run Java_Flink_Versions PreCommit' | ||
| uses: ./.github/actions/rerun-job-action | ||
| with: | ||
| pull_request_url: ${{ github.event.issue.pull_request.url }} | ||
| github_repository: ${{ github.repository }} | ||
| github_token: ${{ secrets.GITHUB_TOKEN }} | ||
| github_job: ${{ github.job }} | ||
| github_current_run_id: ${{ github.run_id }} |
There was a problem hiding this comment.
Please update these steps in accordance with recent changes:
- uses: actions/checkout@v3
- name: Setup repository
uses: ./.github/actions/setup-action
with:
comment_phrase: 'Run Java_Flink_Versions PreCommit'
github_token: ${{ secrets.GITHUB_TOKEN }}
There was a problem hiding this comment.
Changed and confirm the workflow working: https://github.com/Abacn/beam/actions/runs/5755148182/job/15601970982
There was a problem hiding this comment.
LGTM, thanks! Seeing that the run has failed due to checkstyle errors, not sure this is related to your changes
|
that's strange, same command (checkStyle) passed locally and also this workflow passed before |
|
Anyways, now disabled spotless and checkStyle check. We actually should do this because these are split from the main Java PreCommit, where those checks are also disabled. There is a Spotless PreCommit dedicated to run spotless and checkStyle check |
* Setup Java Flink runner PreCommit * Split Flink 1.13-1.15 tests from main Java PreCommit * Only run one flink version on main java precommit * address comments * set timeout * address comment * address comments * Fix copy-paste * Disable spotless and checkstyle
Please add a meaningful description for your change here
Currently, Flink tests are exercised 5 times on Java PreCommit test suite. Some tests are known to be flaky (e.g. #21333). By only run on the lowest and highest supported flink version, it would reduce the flakiness by (1-x)^3 - (1-x)^5 where x is the flaky probability of a single flink test
Other flink versions are still exercised on the new GitHub Action test suite.
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.mdwith 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.