Repository navigation
ci: cut Docker test job from 40m to 25m by building a lean distribution - #20297
Conversation
There was a problem hiding this comment.
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.
Pull request overview
Reduces Docker CI job time by introducing a dedicated Maven build-output cache and avoiding redundant Maven reactor rebuild work during Docker test runs.
Changes:
- Restore/save Maven build-cache outputs (
~/.m2/build-cache) in the Docker tests workflow, saving only on trustedmasterpushes. - Remove
-amfrom the Docker test Maven invocation to avoid rebuilding upstream reactor modules afterbuild-dist.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .github/workflows/docker-tests.yml | Adds restore/save steps for a separate Maven build-output cache around build-dist. |
| .github/scripts/run_docker-tests | Removes -am from the Maven invocation and documents reliance on artifacts installed by build-dist. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
Reviewed 2 of 2 changed files.
Static new-review mode. One high-confidence cache/lifecycle finding is attached inline.
This is an automated review by Codex GPT-5.6-Luna(max)
…to packaging-check The Docker test job spent 18 minutes in build-dist because it ran the full release build (apache-release, rat, and all static checks). Log analysis of a master run shows that cyclonedx (13 CPU-min), javadoc (7.7), the license dependency reports (4), and checkstyle (5) dominated the build, and the distribution module ran alone for the last 5.6 minutes. None of that is needed to produce the tarball for the Docker image, and all of it is already validated on the same commit by the Static Checks CI workflow. - build-dist: build with dist,bundle-contrib-exts,skip-static-checks,skip-tests, the same flags as the Dockerfile builder stage. - packaging-check.sh: enable apache-release (with gpg and dependency-check skipped) so javadoc, source jars, the source-release assembly and the license dependency reports keep a CI run. This job is off the critical path. - static-checks-maven.sh: comment out license_checks_script.sh since RAT and the license checks now run in packaging-check. - run_docker-tests: drop -am; build-dist has already installed the reactor in the same job, and -am re-ran 38 modules including web-console.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
Reviewed 6 of 6 changed files.
Updated-since-review mode: the current head was checked from the incremental diff, then against the full diff and surrounding Maven/CI configuration. Two concrete findings are attached inline.
This is an automated review by Codex GPT-5.6-Luna(max)
…tatic checks - run_docker-tests: restore -am. On a Maven build-cache hit the cached segment includes the install execution, so restored modules such as web-console are not published to ~/.m2/repository; -am resolves them from the reactor. - static-checks-maven.sh: keep a standalone repo-wide apache-rat:check. The packaging-check job excludes benchmarks from its reactor, so it alone would leave that module without license-header validation. Only the license dependency report generation stays delegated to packaging-check.
The build-cache restore/save steps are hard to verify from a PR branch since the cache is only written on master pushes. Remove them and keep the Docker job speedup that is verifiable: the lean build-dist. With no persisted build cache, build-dist always installs every reactor module on the fresh runner, so run_docker-tests can build embedded-tests alone without -am.
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the updated code for correctness, edge cases, concurrency, security, and integration risks; no new issues found.
The incremental changes remove the persisted Docker build cache, keep the Docker test invocation dependent on the preceding full reactor install, and restore repo-wide RAT coverage including benchmarks. The prior review findings are addressed.
Reviewed 5 of 5 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
…d-dist build-dist was only used by the Docker test workflow (it was validate-dist before apache#20270) and the name suggests a general-purpose or release build, which it no longer is. Inline the single mvn command into docker-tests.yml with an explanatory comment and delete the script.
Nothing in the distribution or the Docker tests depends on druid-benchmarks.
No Docker test uses the console, the router tolerates missing console assets (the unit-test shards already run embedded routers with web.console.skip=true), and packaging-check still builds and packages the console on every commit. web-console was the critical path of the distribution build (3m49s of 5m04s).
9294ca5 to
05d007f
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in this review. The lean Docker build still installs the reactor artifacts required by embedded-tests, and the skipped console and benchmarks are not consumed by the Docker image or its tagged tests. The release-only and license checks remain covered by the packaging-check and static-checks jobs.
Reviewed 6 of 6 changed files.
Validation: gh pr checks 20297 --repo apache/druid passed for head 05d007f0e174855cd32e9af907199dc3d569d938; no local build or broad test run was performed.
This is an automated review by Codex GPT-5.6-Luna(max)
Summary
Cut the Docker test job from 39m41s to 25m06s by building only what the Docker image needs, and move the release-only validation it used to perform into the
packaging-checkjob, which is off the CI critical path.Observed Docker baseline
Baseline is the
masterDocker job in workflow run 34300307586, job 102305613946, at commit cc8b586. The job completed successfully in 39m41s:Attributing the Maven goals in the distribution-build log to CPU time (45 CPU-minutes on a 4-vCPU runner):
cyclonedx:makeBom(118 modules)javadoc:jar(79 modules, fromapache-release)exec:exec(of whichgenerate-licenses-reportindistribution: 229s)checkstyle:checkweb-consolenpm install + buildassembly:single(binary 36s, source release 42s)compile+testCompileapache-rat:checkThe
distributionmodule also ran alone for the last 5m36s of the build (three of four cores idle), almost entirely in theapache-release-onlygenerate-licenses-reportexecution and the source-release assembly.In the test step,
-pl embedded-tests -amre-ran 38 upstream modules (includingweb-console, ~2 minutes) before the first test started.All of the removed work is already validated on the same commit by the Static Checks CI workflow:
packaging-checkbuilds the distribution with-Prat -Pdist -Pbundle-contrib-extsand runs checkstyle, pmd, forbiddenapis, animal-sniffer, enforcer and cyclonedx;static-checks-mavenruns the same static checks plus spotbugs. The only checks that had no other CI run were theapache-release-only ones: javadoc generation, source jars, the source-release assembly and the license dependency reports.Changes
.github/workflows/docker-tests.yml: build the distribution inline withmvn -B -T1C clean install -pl '!benchmarks' -Pdist,bundle-contrib-exts,skip-static-checks,skip-tests -Dweb.console.skip=true, the same profiles the Dockerfile builder stage uses.benchmarksis excluded since nothing in the distribution or the Docker tests depends on it (no measurable time saving; it ran on an idle thread). The web console assets are skipped: no Docker test uses the console, the router tolerates the missing assets (the unit-test shards already run embedded routers withweb.console.skip=true), andpackaging-checkstill builds and packages the console with-Dweb.console.skip=falseon every commit.web-consolewas the critical path of the lean build (3m49s of 5m04s), so this should bring the step to roughly 4 minutes. The image under test therefore differs from the shipped image only in lacking the console assets. Dropsapache-release,ratand all static checks from the Docker job..github/scripts/build-distis deleted: the Docker workflow was its only caller and the name suggested a general-purpose release build..github/scripts/packaging-check.sh: add-Papache-release -Dgpg.skip -Ddependency-check.skipto both Maven invocations, so javadoc, source jars, the source-release assembly and the license dependency reports keep a CI run. This job is not on the critical path of its workflow..github/scripts/static-checks-maven.sh: replacelicense_checks_script.shwith a standalone repo-wideapache-rat:check -Prat. RAT stays here becausepackaging-checkexcludesbenchmarksfrom its reactor; the license dependency report generation andcheck-licenses.pyare now covered bypackaging-checkviaapache-release. Also remove the duplicatedistributioninstall..github/scripts/run_docker-tests: drop-am. The distribution build step installs every reactor module on the fresh runner and no Maven build cache is persisted across jobs, soembedded-testsresolves its dependencies (including the test-scopedweb-console) from~/.m2/repository. If a persisted build cache is ever reintroduced,-ammust come back ormaven-install-pluginmust be forced viarunAlways..github/scripts/openrewrite.sh: remove the duplicatedistributioninstall.The only workflow change is the inlined build step; no caching or job-graph changes.
Distribution-install cleanup rationale
The removed commands in
static-checks-maven.shandopenrewrite.share:They only install the default
distributionproject without the packaging profiles, and neither script consumes the result.packaging-check.shremains the job that builds and validates the distribution.Measured outcome
PR head 05d007f, Unit & Integration run 34428720842 and Static Checks run 34428720600:
packaging-check(now runsapache-release)static-checks-maven(license reports removed, RAT kept)All 107 Docker tests pass (19 skipped, as on master). The Docker job is no longer the longest job in the Unit & Integration workflow; the unit-test shards (~35–37 min) are now the critical path.