ci: retry the Maven distribution download in every job that runs mvnw - #5881
ci: retry the Maven distribution download in every job that runs mvnw#5881andygrove wants to merge 1 commit into
Conversation
The retry and cache added in apache#5422 only covered callers of the java-test action. Every other Maven caller (lint-java, build-spark-4-1, the benchmark verify jobs, setup-spark-builder, the Iceberg installs, preflight's RAT check) still downloads apache-maven-3.9.6-bin.zip on a cache miss with no retry, so one 403 from Maven Central fails the job before it builds anything. Move the restore/retry/save block into a bootstrap-maven composite action and call it at the end of setup-builder and setup-macos-builder, which every ./mvnw caller runs first, plus directly from preflight. Route edits to the new action to the same jobs as the setup actions.
sunchao
left a comment
There was a problem hiding this comment.
Correctness
The distribution retry previously lived inside java-test, leaving lint, preflight and other Maven callers exposed to an initial download failure. This PR extracts the same three restore/bootstrap/save steps into bootstrap-maven, invokes it after Java setup in the two shared builder actions, and adds a direct preflight call before RAT. No Spark expression, operator, native implementation or build profile changes.
The extraction preserves both cache-step definitions and the retry script apart from its terminal message. The remaining java-test steps are unchanged. I traced the wrapper callers, including the nested Spark builder, Rust prerequisite and Delta gate paths. They have checkout and Java setup before bootstrap. The new action reaches the ten existing build routes, while event policy stays unchanged.
There is one P2 finding on the new preflight caller: its distribution cache cannot be saved because the copied path list includes an inaccessible root-home directory. The inline comment includes the current CI failure and the requested correction. The retry loop itself still works.
Validation
The repository CI checker passed locally. A separate YAML audit verified the extraction and caller ordering. Running the exact retry shell body against a local wrapper test double verified success on attempts one through four, and failure after four failed attempts with the expected three backoff intervals. This tests control flow and arguments, not real HTTP failures.
The current checks report 57 successes and 10 skips. I inspected the preflight, Spark 4.0/JDK 21 lint and Spark 4.1 expression-job logs. All checked out f7effb02, whose parents are the assigned base and head and whose full tree equals head 57ecfd42. Preflight runs bootstrap and passes RAT, the CI checker and actionlint. The container jobs restore Maven 3.9.6 and pass their subsequent work. The expression job reports 1,382 successful ScalaTest cases with zero failures. Preflight's green result includes the cache-save warning described above.
I ran no local Spark/native/JNI suite or benchmark. The macOS and queue-only workflow executions were skipped in this PR run. This CI-only change does not require a new Spark semantic comparison, and the unavailable maintained Spark 3.4/4.1 branches are not counted as source coverage.
Performance
The retry remains confined to --version, so it cannot repeat compilation or tests. It stops on the first success and allows at most three sleeps totaling 70–82 seconds, excluding command duration. Keeping the distribution cache independent of POM changes avoids needless invalidation. The shared setup now adds a cache/version check to native-only producer jobs too, but does not repeat a build.
The preflight cache-path finding matters here as well: the valid downloaded distribution is discarded with the runner instead of being saved for reuse. The inspected container cache hits demonstrate reuse in that environment. They do not establish a preflight cache benefit or a measured reduction in CI failure rate.
Design
Centralizing bootstrap at the shared setup boundary covers existing callers without copying a retry loop into every workflow. Running it after JDK installation preserves its prerequisite, and keeping the preflight call explicit fits that job's smaller setup. The remaining adjustment is to choose cache paths that work for both hosted preflight and container jobs, with the same selection used for restore and save.
Abstraction & complexity
A small composite action is appropriate for the repeated behavior. It adds no configurable retry framework, and java-test loses the duplicate implementation. The ten routing additions and regression case make changes to this shared action visible to the build tier. Apart from the concrete cache-path finding, I found no additional correctness, performance or complexity issue requiring a change.
| ~/.m2/wrapper/dists | ||
| /root/.m2/wrapper/dists |
There was a problem hiding this comment.
Correctness
[P2] Use an accessible distribution cache path in preflight
Could we exclude the inaccessible /root/.m2/wrapper/dists path for the new non-container preflight caller? In this PR's Preflight log, Maven is installed under /home/runner/.m2/wrapper/dists, but Save Maven distribution emits EACCES: permission denied, lstat '/root/.m2/wrapper/dists' and saves no cache. The cache action aborts path enumeration on this error, so the valid user-home directory is not saved either, even though the step stays green. This leaves the pipeline's preflight gate downloading Maven again on a cache miss and misses the caching part of the reliability fix. Please select accessible distribution paths for each runner environment in both restore and save, then verify a cold save and warm restore on ubuntu-slim as well as the container path.
Which issue does this PR close?
N/A. CI reliability follow-up to #5422.
Rationale for this change
The
Lint Java (Spark 4.0, JDK 21)job on #5839 (log) failed one minute in, before compiling anything:#5422 added a retry and a distribution cache for exactly this failure, but only inside the shared
java-testaction. Every other Maven caller still does an unguarded first./mvnwon a fresh runner:lint-java,build-spark-4-1, both benchmark verify jobs,setup-spark-builder(viamake release), the three Icebergmvnw installsteps,rust-test,pyarrow-udf,delta-build-gate, and preflight's RAT check. Any of them turns a transient Maven Central 403/429 into a red job that says nothing about the patch.What changes are included in this PR?
.github/actions/bootstrap-maven, holding the restore / retry / save block that used to live inline injava-test. Same behavior as before: up to four attempts with 10s/20s/40s backoff plus jitter, cache keyed onmaven-wrapper.properties, cache disabled on macOS per the existing runner-images workaround.setup-builderandsetup-macos-buildercall it as their last step (after the JDK is installed). Every job that runs./mvnwgoes through one of those two actions, so this covers all the callers listed above in one place.preflightinci.ymlcalls it directly before the RAT check, since that job does not usesetup-builderand gates everything else.java-testdrops its inline copy; its callers already run a setup action first, so the distribution is installed by the time it runs.dev/ci/compute-changes.pyroutes edits to the new action to the same jobs as the setup actions, anddev/ci/check-ci-config.pypins that with a routing case.This is the first place a local composite action uses another local composite action (
setup-builder->bootstrap-maven); a comment injava-testthat called that pattern untested is updated.How are these changes tested?
python3 dev/ci/check-ci-config.pypasses with the new routing case.actionlint --shellcheck=offpasses on the workflows.mvnwthat fails twice then succeeds (returns 0 after three attempts, two warnings with delays in the 10-14s and 20-24s ranges) and against one that always fails (exits 1 after four attempts with the::error::line).bootstrap-maven/**route runs the build tier.