Copilot commented on code in PR #3880:
URL: https://github.com/apache/celeborn/pull/3880#discussion_r4313821663
##########
.github/workflows/maven.yml:
##########
@@ -36,6 +36,12 @@ jobs:
java-version: ${{ matrix.java }}
cache: maven
check-latest: false
+ - name: Cache Maven distribution
+ uses: actions/cache@v4
+ with:
+ path: build/apache-maven-*
+ key: maven-distribution-${{ hashFiles('pom.xml') }}
+ restore-keys: maven-distribution-
Review Comment:
The cache key doesn’t include OS/arch context (e.g., `${{ runner.os }}` /
`${{ runner.arch }}`). If this workflow is ever run on different runner
platforms, the restore key prefix can cause a cache hit for an incompatible
Maven distribution layout or permissions, leading to flaky behavior. Including
OS/arch in the key (and keeping restore keys scoped similarly) makes the cache
safer and more predictable.
##########
.github/workflows/maven.yml:
##########
@@ -98,6 +104,12 @@ jobs:
java-version: ${{ matrix.java }}
cache: maven
check-latest: false
+ - name: Cache Maven distribution
+ uses: actions/cache@v4
+ with:
+ path: build/apache-maven-*
+ key: maven-distribution-${{ hashFiles('pom.xml') }}
+ restore-keys: maven-distribution-
Review Comment:
The same cache step is duplicated across multiple jobs. To reduce the chance
of future drift (e.g., needing to update the key/path in several places),
consider factoring it via a YAML anchor/alias or moving it into a reusable
workflow/composite action and referencing it from each job.
##########
.github/workflows/maven.yml:
##########
@@ -36,6 +36,12 @@ jobs:
java-version: ${{ matrix.java }}
cache: maven
check-latest: false
+ - name: Cache Maven distribution
+ uses: actions/cache@v4
+ with:
+ path: build/apache-maven-*
+ key: maven-distribution-${{ hashFiles('pom.xml') }}
Review Comment:
The cache key is tied to `pom.xml`, which will invalidate the Maven *binary*
cache whenever dependencies/config change, even though the Maven distribution
likely hasn’t. This reduces cache hit rate and may cause unnecessary
re-downloads. Prefer keying off the Maven distribution version source (e.g.,
the script/config that pins the Maven version, such as `build/mvn` or a
dedicated version file) rather than `pom.xml`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
