Skip to content

[CELEBORN-2488] Cache Maven distribution in CI workflows - #3880

Closed
yew1eb wants to merge 2 commits into
apache:mainfrom
yew1eb:CELEBORN-2488
Closed

yew1eb wants to merge 2 commits into
apache:mainfrom
yew1eb:CELEBORN-2488

Conversation

@yew1eb

@yew1eb yew1eb commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Each job in maven.yml re-downloads the Maven distribution from archive.apache.org via build/mvn (~25s typical, 1m49s observed on a slow network), even though ~/.m2 is already cached by setup-java. This PR adds an actions/cache@v4 step for the extracted Maven install under build/, so build/mvn reuses the cached binary — its install_app already skips the download when apache-maven-<version>/bin/mvn exists.

This mirrors the sbt launcher caching added for sbt.yml in CELEBORN-2432.

Why are the changes needed?

Save ~25s–2min per job; ~10–40 minutes of total machine time per CI run.

Does this PR resolve a correctness bug?

  • Yes

Does this PR introduce any user-facing change?

  • No

How was this patch tested?

Verified locally that build/mvn skips the download when build/apache-maven-<version>/bin/mvn is present. CI on this branch shows the cache step restoring and no curl download of the Maven tarball.

@cxzl25
cxzl25 requested a balanced review from Copilot October 19, 2026 12:14
@yew1eb yew1eb closed this Oct 19, 2026
@yew1eb yew1eb reopened this Oct 19, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open issues
What changed in this PR

Adds GitHub Actions cache entries for the downloaded/extracted Maven distribution so CI jobs can reuse build/apache-maven-* instead of re-downloading Maven each run.

Changes:

  • Add actions/cache@v4 step to cache build/apache-maven-* in each Maven CI job
  • Use a shared cache key prefix (maven-distribution-) with a pom.xml hash-based key and restore keys
File Description
.github/​workflows/​maven.yml Adds caching for the extracted Maven distribution under build/ to speed up CI runs

Review effort: Lite

Comment thread .github/workflows/maven.yml Outdated
Comment thread .github/workflows/maven.yml Outdated
Comment thread .github/workflows/maven.yml Outdated
Follow-up to the Copilot review feedback:

- Include runner.os in the cache key and restore key so the cache can
  never be reused across different runner platforms.
- Keep keying on pom.xml: it is where <maven.version> is defined and
  build/mvn reads it from, so version bumps invalidate the exact key;
  unrelated pom changes still hit the restore key prefix, so the
  distribution is not re-downloaded.

The per-job duplication is kept as-is for readability; factoring it into
a composite action can be a follow-up if needed.

JIRA: https://issues.apache.org/jira/browse/CELEBORN-2488
@yew1eb

yew1eb commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Addressed in 0e286a6:

  • OS/arch in key: added runner.os to both the cache key and the restore key prefix, so the cache can never be reused across runner platforms.
  • pom.xml as the key source: kept intentionally — pom.xml is where <maven.version> is defined and where build/mvn reads it from, so a Maven version bump invalidates the exact key. Unrelated pom changes fall back to the restore key prefix, and since the restored cache already contains build/apache-maven-<version>/, build/mvn skips the download, so there is no re-download cost.
  • Duplication across jobs: kept the steps inlined for readability; happy to factor them into a composite action as a follow-up if reviewers prefer.

@codecov

codecov Bot commented Oct 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.59%. Comparing base (90954f7) to head (0e286a6).
⚠️ Report is 61 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3880      +/-   ##
============================================
+ Coverage     58.74%   60.59%   +1.85%     
- Complexity      231      324      +93     
============================================
  Files           399      401       +2     
  Lines         28058    28415     +357     
  Branches       2740     2793      +53     
============================================
+ Hits          16479    17214     +735     
+ Misses        10380     9886     -494     
- Partials       1199     1315     +116     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zaynt4606 zaynt4606 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.

LGTM

@zaynt4606 zaynt4606 closed this in 60a90e5 Oct 20, 2026
@zaynt4606

Copy link
Copy Markdown
Contributor

Merge Summary:

Posted by merge_pr.py

@zaynt4606

Copy link
Copy Markdown
Contributor

Thanks @yew1eb for the contribution!
Merged to main(v1.0.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants