Skip to content

ci: retry the Maven wrapper bootstrap in every job that calls ./mvnw directly - #5852

Open
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:ci-maven-bootstrap-retry
Open

ci: retry the Maven wrapper bootstrap in every job that calls ./mvnw directly#5852
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:ci-maven-bootstrap-retry

Conversation

@andygrove

@andygrove andygrove commented Sep 10, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

No issue — found while investigating a CI failure on #5850.

Rationale for this change

Verify TPC-H Results failed on #5850 at its Build project step, 105 seconds in and before anything was compiled. The check-run annotation names the cause:

https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.6/apache-maven-3.9.6-bin.zip

That's ./mvnw downloading the Maven distribution itself — not a dependency, not a test. Nothing to do with TPC-H, and nothing to do with the PR it failed on (which only touched CI config).

We already handle this. ./.github/actions/java-test caches the distribution under ~/.m2/wrapper/dists and retries ./mvnw --version four times with exponential backoff. The problem is that five jobs in pr_build_linux.yml don't go through java-test — they call ./mvnw directly — so they had neither the cache nor the retry:

Job First Maven use
lint-java scalafix check
build-spark-4-1 compile, skip tests
celeborn-reflection-compatibility reflected-internals check
verify-benchmark-results-tpch the job that failed
verify-benchmark-results-tpcds same shape as TPC-H

The workflows README claims this failure mode is handled. It is, but only for jobs routed through java-test.

This also matters for #5838: under a merge queue these jobs gate the queue, so a bootstrap blip would block every merge rather than costing one PR a re-run.

What changes are included in this PR?

Extracts the restore/retry/save sequence into ./.github/actions/maven-bootstrap and calls it from all five jobs before their first Maven use.

Registers the new action in the Linux change filter (FILTERS['build_linux'] in dev/ci/compute-changes.py) and pins that routing with a case in dev/ci/check-ci-config.py. Per @sunchao's review: without it, a later edit confined to .github/actions/maven-bootstrap/** routes to nothing — the changes gate reports build_linux=false, ci.yml skips the whole Linux workflow, and the edit merges without any of the five consumers having run it. This PR didn't expose the gap only because it also edits pr_build_linux.yml.

java-test keeps its inline copy rather than calling the new composite. A local action invoking another local action is deliberately avoided in this repo — the README already says as much about the artifact-upload wrapper — and converting java-test would be a bigger, riskier change than the bug warrants.

The README wording is corrected to say which jobs are actually covered.

How are these changes tested?

CI on this PR is the test: all five jobs exercise the new step, and lint-java, build-spark-4-1 and celeborn-reflection-compatibility are the fast ones that will show it working within a few minutes.

Locally I verified with a script over the parsed workflow that every job invoking ./mvnw now either uses java-test or has maven-bootstrap at a lower line number than its first Maven call — all five report OK, none regress.

For the routing: with .github/actions/maven-bootstrap/action.yaml as the only changed file, EVENT_NAME=pull_request dev/ci/compute-changes.py now reports build_linux=true and every other output false; drop the filter line and it goes back to false, and check-ci-config.py fails with exactly that. check-suites.py, check-benchmark-runner.py, test-iceberg-shards.py, check-ci-config.py, actionlint, apache-rat:check and prettier --check "**/*.md" are all clean.

The retry path itself can't be exercised without an actual network failure; the logic is copied verbatim from java-test, where it has been in use already.

Rebased onto main to pick up #5849 — the five red [expressions] jobs on the previous CI run were the pre-existing CometCodegenSuite decimal-promotion breakage from #5610, not anything in this PR. Those jobs go through java-test, which this PR doesn't touch.

Notes for reviewers

Same gap exists in ci.yml's RAT check and the direct ./mvnw calls in pr_benchmark_check.yml, pyarrow_udf_test.yml and iceberg_spark_test_reusable.yml. I left those out to keep this reviewable against the failure that prompted it — happy to extend it if you'd rather do them all at once. ci.yml's preflight is arguably the most valuable of those, since it gates the whole pipeline.

@github-actions github-actions Bot added build Build environment enhancement New feature or request area:ci CI/CD, GitHub Actions, build tooling labels Sep 10, 2026
@andygrove
andygrove requested a review from comphead September 10, 2026 19:30

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andygrove makes sense to me

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness

The new action gives the five direct Maven callers a distribution cache and a bounded startup retry before their existing commands. All five caller sequences place it after Java setup and before the first Maven use. The authored diff changes no Spark expression or operator implementation, so it does not change maintained Spark null, ANSI, overflow or fallback semantics. This review does not add maintained-version compatibility coverage.

I found one P2 integration issue in the inline comment: the new action is absent from the Linux change filter. The current PR runs Linux CI because it also edits the workflow, but a later action-only edit would skip its consumers.

The exact retry shell passed five local cases with mocked Maven and sleep, covering immediate success, success after one through three failures, and exhaustion after four failures. These are shell-control tests, not a real download-failure reproduction or local Spark execution. CI used merge dd984ed6, whose parents are the assigned base and head and whose full tree equals head ef823b74. All nine matrix jobs covering the five new caller types passed. I inspected bootstrap logs for one job of each type, including cache hits and a cold bootstrap. The five failed expression jobs report CometCodegenSuite's decimal-promotion coverage assertion after Maven started. Those jobs still use the unchanged inline action. Overall CI is not green. CI run.

Performance

The retry is outside compilation and test execution, and the distribution cache is independent of dependency POM changes. Four sampled cache-hit jobs spent about 0.74 to 2.02 seconds restoring the cache and 0.18 to 0.24 seconds starting Maven. These are CI observations, not a before-and-after benchmark. Persistent failure adds 70 to 82 seconds of scheduled backoff across three waits, plus the duration of four attempts. No query hot path changes warrant an expression microbenchmark.

Design

Keeping startup recovery separate from build and test commands avoids rerunning failed tests or partially completed builds. Saving immediately after successful startup allows the cache to survive a later job failure. The change-filter registration should accompany the extraction so this new workflow dependency remains exercised when edited on its own.

Abstraction & complexity

One small composite replaces five potential copies without adding configuration inputs or another retry framework. Its cache steps match the existing java-test steps, and its shell differs only in the final failure message. Keeping the inline copy respects the repository's stated choice to avoid nested local actions, with the tradeoff that the two copies must remain consistent.

${{ runner.os }}-java-maven-

- name: Bootstrap Maven
uses: ./.github/actions/maven-bootstrap

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness

[P2] Route changes to the new bootstrap action into Linux CI

Could you add .github/actions/maven-bootstrap/** to FILTERS['build_linux'] in dev/ci/compute-changes.py and add a routing case in dev/ci/check-ci-config.py? With only .github/actions/maven-bootstrap/action.yaml as input, the current filter returns build_linux=false. The ci.yml gate then skips the entire Linux workflow, including all five consumers of this new action, so an action-only change can pass preflight without exercising the bootstrap. This PR's CI run does not expose the gap because the accompanying edit to pr_build_linux.yml makes the filter true. Filter at the reviewed head.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed. Added .github/actions/maven-bootstrap/** to FILTERS['build_linux'] and pinned it with a routing case in check-ci-config.py:

# The Maven bootstrap composite is called only from pr_build_linux.yml.
([".github/actions/maven-bootstrap/action.yaml"], {"build_linux"}),

With that file as the only change, EVENT_NAME=pull_request compute-changes.py now reports build_linux=true and everything else false. I checked the routing case actually bites by deleting the filter line again — check-ci-config.py fails with exactly your finding.

On the red [expressions] jobs: those are #5610's CometCodegenSuite decimal-promotion breakage, fixed on main by #5849 after this branch's base. I've rebased, so the re-run should be clean.

…directly

`Verify TPC-H Results` failed on apache#5850 at its `Build project` step, 105 seconds
in and before anything was compiled. The check-run annotation gives the cause:

  https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.6/apache-maven-3.9.6-bin.zip

That is `./mvnw` downloading the Maven distribution itself, not a dependency
and not a test.

`./.github/actions/java-test` already handles this: it caches the distribution
under `~/.m2/wrapper/dists` and retries `./mvnw --version` four times with
exponential backoff. But five jobs in pr_build_linux.yml never go through
java-test -- they invoke `./mvnw` directly -- so none of them had either the
cache or the retry:

  lint-java                          scalafix check
  build-spark-4-1                    compile, skip tests
  celeborn-reflection-compatibility  reflected-internals check
  verify-benchmark-results-tpch      the job that failed
  verify-benchmark-results-tpcds     same shape as TPC-H

Extract the cache/retry/save sequence into `./.github/actions/maven-bootstrap`
and call it from all five before their first Maven use. java-test keeps its
inline copy: a local action invoking another local action is deliberately
avoided in this repository, and the workflows README already says so about the
artifact-upload wrapper.

Register the new action in the Linux change filter. Without it, a later edit
confined to `.github/actions/maven-bootstrap/**` routes to nothing: the
`changes` gate reports `build_linux=false`, ci.yml skips the whole Linux
workflow, and the edit merges without any of the five consumers having run it.
This PR does not expose the gap, because it also edits pr_build_linux.yml. Pin
the routing with a case in dev/ci/check-ci-config.py so a filter deletion
cannot pass unnoticed either.

The workflows README claimed this failure mode was handled. It was, but only
for jobs routed through java-test; the wording is corrected.

Worth noting for apache#5838: under a merge queue these jobs gate the queue, so this
failure mode would block every merge rather than costing one PR a re-run.

Follow-up, not covered here: `ci.yml`'s RAT check and the direct `./mvnw` calls
in `pr_benchmark_check.yml`, `pyarrow_udf_test.yml` and
`iceberg_spark_test_reusable.yml` have the same gap. Left out to keep this
reviewable against the failure that prompted it.
@andygrove
andygrove force-pushed the ci-maven-bootstrap-retry branch from ef823b7 to 9de7b1d Compare September 12, 2026 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ci CI/CD, GitHub Actions, build tooling build Build environment enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants