HBASE-29838 Run Hadoop Check as a GitHub Action#7651
HBASE-29838 Run Hadoop Check as a GitHub Action#7651ndimiduk wants to merge 2 commits intoapache:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Looks like we occupied the runner for 6h and then it was aborted. |
|
https://infra.apache.org/github-actions-policy.html The policy here does not say about the 6 hours timeout... We can ask infra about the rules and the size of the github runners, our jenkins runners finished in "343m 23s", which was very close to 6 hours, so if the machine of the github runner is weaker, the build will be very easy to cost more than 6 hours... |
|
My action itself has |
|
or not. 6h is GH's hard limit, https://docs.github.com/en/actions/reference/limits |
|
Then maybe we should try self hosted github runners? For self hosted runners the execution time limit is 5 days... |
1829212 to
352a9a1
Compare
Yes we should bring this back to our CI discussions with Infra. Maybe we can borrow from the pool of new Jenkins workers while we continue to build this out. Yetus is supposed to provide smart, selective detection of module changes when it decided which tests to run. I think the new .github directory broke that for this run, so I've pushed a change to exclude it, maybe that will help. I'm also going to see if I can manually parallelize the unit test runs -- maybe break out three separate checks for the three main unit test groups or something like that. |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Maybe we could split the UTs run as a seperated github check? Or even more, we could split the UTs run as two seperated check, one has -PrunDevTests(for small/medium tests) and one has -PrunLargeTests(for large tests). |
Yep, that's exactly my thinking as well. Landing these other cleanup issues and I'll be back. |
352a9a1 to
adfd056
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
Okay this is better. Module selection chose only hbase-examples for running the unit tests. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Looking over out last successful nightly on master, the Large tests on hbase-server still took 7h. I'm looking for other ways to partition this up. |
adfd056 to
ad51411
Compare
This comment has been minimized.
This comment has been minimized.
| cd "${{ github.workspace }}" | ||
| bash src/dev-support/jenkins_precommit_github_yetus.sh | ||
|
|
||
| - name: Publish Test Results |
There was a problem hiding this comment.
I think here we should also include a job summary, but do not like other checks, as there is only one unit plugin, showing the result of yetus does not provide more information.
I think we should filter the junit console output, filter out the failed UTs and show them on the job summary page. If no failure, we just show something like 'no failure' or 'pass'.
There was a problem hiding this comment.
I originally intended to add it later, but yeah, let me look into this.
There was a problem hiding this comment.
We can do it later, the current one is OK for now.
There was a problem hiding this comment.
I added it but it seems it's not working. I don't see any test reports on the Summary for the failed jobs...
| pip install -q -r dev-support/test-waves/requirements.txt | ||
|
|
||
| # Fetch wave file - returns path to either GHA artifact cache or committed fallback | ||
| WAVE_FILE=$(python dev-support/test-waves/fetch-wave-files.py "${{ matrix.wave }}" "${{ github.base_ref }}") |
There was a problem hiding this comment.
I think here we could upload the wave files to nightly, which is easier for committers to fix the files manually, and downloading from nightlies does not need any credentials.
And for this job, I prefer we just run it on jenkins, which is easier for us to upload things to nightles, and it does not cost too many resources.
|
On the small medium tests, I think this is the problem... Let me check the profile config about parallelism... |
|
We do have some medium tests which need several minutes to finish. I downloaded the artifacts for small-medium and do some calculating, the total execution time for all the tests is The execution time for tests which run for more than 90 seconds is For more than 60 seconds is And what makes things worse is that, for -PrunLargeTests, all large tests are run in firstPartGroup, where the forkCount is 1C. But for -PrunDevTests, the medium tests are run in secondPartGroup, where the forkCount is 0.5C. So here, I propose that, first we promote all the tests which run for more than 90 seconds to large tests, and then split small-medium to small and medium, use -PrunSmallTests and -PrunMediumTests, in this way we will all use the 1C forkCount to run the tests, which could make them run faster. @ndimiduk WDYT? Thanks. |
2d42df9 to
c256196
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Apache9
left a comment
There was a problem hiding this comment.
Let's also separated runSmallTests and runMediumTests?
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <configuration> | ||
| <includes> |
There was a problem hiding this comment.
Will this be overridden by the command line parameter?
There was a problem hiding this comment.
Okay so what I found is, when using the Category selector + the wave include list + the exclude list, all these work together as you would hope: only Large tests are run, which match the include regex, and entries found in the exclude list are indeed excluded.
However, if the yetus feature that adds -Dtest.include.pattern, i think it'll override the includes defined here. I'm not sure if that feature is used... anyway, I think that I prefer these wave definitions live here in the pom, rather than up in the GHA or personality script or whatever.
|
Seems the include for large wave does not work as expected... |
|
Yeah, pattern matching doesn't support character ranges, or something. Also I noticed that we have some test files named like TestFoo but also FooTest, so addressing that. I did see your note about breaking up the devTests profile. I'll make that change also. |
c256196 to
0c5a250
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I spot-checked the tests run by large waves 1 and 3 (2 is still running) and they look to contain only the expected tests. I think we're good here. Any other comments @Apache9 ? |
This comment has been minimized.
This comment has been minimized.
0c5a250 to
4fbb099
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| GITHUB_PASSWORD: ${{ secrets.GITHUB_TOKEN }} | ||
| GITHUB_USER: ${{ github.actor }} | ||
| PATCHDIR: "${{ github.workspace }}/yetus-jdk17-hadoop3-unit-check/output" | ||
| PLUGINS: "github,htmlout,maven,unit" |
There was a problem hiding this comment.
Do we need maven plugin here? We do not need mvninstall check?
There was a problem hiding this comment.
I dug into this earlier, and the answer seems to be no, the unit plugin has enough to go on, so long as maven plugin is also present. The mvninstall was redundant work from the compile check.
There was a problem hiding this comment.
I mean can we remove maven plugin here? The console output for the unit check showed that we did run mvninstall check...
There was a problem hiding this comment.
I think we need the maven plugin in order for the unit plugin to be able to run maven to run the tests. From reading the yetus code, maven is the "build tool" that is used by the "module worker".
4fbb099 to
571a79d
Compare
This comment has been minimized.
This comment has been minimized.
|
(!) A patch to the testing environment has been detected. |
1 similar comment
|
(!) A patch to the testing environment has been detected. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
No description provided.