Copilot commented on code in PR #20297:
URL: https://github.com/apache/druid/pull/20297#discussion_r3958047375
##########
.github/scripts/run_docker-tests:
##########
@@ -35,4 +35,5 @@ fi
# No snapshot updates
OPTS+=" -nsu"
-mvn -B -pl embedded-tests -am $OPTS verify -Pdocker-tests,skip-static-checks
-DskipUTs -D$DRUID_IMAGE_SYS_PROPERTY=$DRUID_IMAGE_NAME
"-DjfrProfilerArgLine=$JFR_PROFILER_ARG_LINE" "$@"
+# build-dist has already installed the reactor artifacts in the local
repository.
+mvn -B -pl embedded-tests $OPTS verify -Pdocker-tests,skip-static-checks
-DskipUTs -D$DRUID_IMAGE_SYS_PROPERTY=$DRUID_IMAGE_NAME
"-DjfrProfilerArgLine=$JFR_PROFILER_ARG_LINE" "$@"
Review Comment:
This script now assumes `build-dist` has already installed all required
reactor artifacts into the local repository. If this script is invoked in any
context where that prerequisite is not met, removing `-am` will cause build
failures due to missing locally-installed snapshot artifacts. To make the
script robust across callers, consider adding an explicit flag/env var to
disable `-am` only in the workflow path that runs `build-dist`, or add a small
preflight that falls back to `-am` when required artifacts are not present.
##########
.github/workflows/docker-tests.yml:
##########
@@ -31,8 +31,24 @@ jobs:
distribution: 'zulu'
java-version: 25
cache: 'maven'
+ # setup-java caches Maven dependencies in ~/.m2/repository. This separate
+ # cache stores build outputs produced by the Maven build-cache extension.
+ - name: Restore Maven build cache
+ id: maven-build-cache
+ uses: actions/cache/restore@v4
+ with:
+ path: ~/.m2/build-cache
+ key: maven-build-cache-v1-${{ runner.os }}-java25-${{ github.sha }}
+ restore-keys: |
+ maven-build-cache-v1-${{ runner.os }}-java25-
Review Comment:
Using `${{ github.sha }}` in the cache key will create a distinct cache
entry per commit. With `restore-keys` enabled, this pattern tends to steadily
grow the number of caches (and may hit GitHub Actions cache quotas / eviction
churn), while providing limited benefit over a single shared key. Consider
keying the build-output cache off inputs that actually affect build outputs
(e.g., hash of `pom.xml`/relevant build files, plus OS/JDK/cache version), so
multiple commits can reuse the same cache entry.
##########
.github/workflows/docker-tests.yml:
##########
@@ -31,8 +31,24 @@ jobs:
distribution: 'zulu'
java-version: 25
cache: 'maven'
+ # setup-java caches Maven dependencies in ~/.m2/repository. This separate
+ # cache stores build outputs produced by the Maven build-cache extension.
+ - name: Restore Maven build cache
+ id: maven-build-cache
+ uses: actions/cache/restore@v4
+ with:
+ path: ~/.m2/build-cache
+ key: maven-build-cache-v1-${{ runner.os }}-java25-${{ github.sha }}
+ restore-keys: |
+ maven-build-cache-v1-${{ runner.os }}-java25-
- name: Build the Druid distribution
run: .github/scripts/build-dist
+ - name: Save Maven build cache
+ if: ${{ github.event_name == 'push' && github.ref ==
'refs/heads/master' && steps.maven-build-cache.outputs.cache-hit != 'true' }}
+ uses: actions/cache/save@v4
+ with:
+ path: ~/.m2/build-cache
+ key: maven-build-cache-v1-${{ runner.os }}-java25-${{ github.sha }}
Review Comment:
The `cache-hit` output is only `true` for an exact key match; when the cache
is restored via `restore-keys`, `cache-hit` remains `false`. With the current
`if:` condition, this will save a new cache on every `master` push even when a
compatible cache was successfully restored via prefix, which is likely
unintended and amplifies cache growth. Consider gating the save on whether any
cache was matched (e.g., using the restore step’s matched-key output) and/or
switching to a stable key so exact-hit behavior is meaningful.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
