slachiewicz commented on code in PR #882:
URL: https://github.com/apache/incubator-xtable/pull/882#discussion_r3745505208
##########
.github/workflows/mvn-ci-build.yml:
##########
@@ -43,5 +43,25 @@ jobs:
distribution: 'temurin'
cache: maven
+ # `cache: maven` above only persists ~/.m2/repository. The build cache
+ # extension configured in .mvn/extensions.xml writes to
~/.m2/build-cache,
+ # so without this it starts empty on every run and never restores a
module.
+ # Only `main` writes an entry, so pull requests warm up from `main`
instead
Review Comment:
Coming back to this properly — and measuring it turned up a real problem
with the PR as I first posted it, so thanks for pushing on it. Both parts
below, and I've just pushed a commit that addresses them.
**How invalidation works.** The extension computes a checksum per module
from (1) the effective POM — so dependency versions, plugin versions and plugin
configuration are all covered, (2) a content digest of every file under the
module's source, resource, test-source and test-resource directories, plus
paths referenced from plugin configurations, and (3) for dependencies on other
modules in this reactor, *that module's own checksum*. Point 3 is the
propagation mechanism, and it's the direct answer to your question: a PR
touching `xtable-core` produces new checksums for core **and every module
downstream of it**, so all of those rebuild and re-run their tests from
scratch. Only a module whose complete input set matches a main build is
restored — and what gets restored is literally the jar a green main run
produced, since only the `push`-to-main job writes the cache and GitHub scopes
any cache a PR creates away from main.
What the checksum does *not* cover, honestly: external release dependencies
are trusted by GAV rather than by content (standard Maven behaviour); the JDK
isn't hashed (single fixed JDK here, and I've now put it in the Actions cache
key); and command-line flags aren't hashed. That last one is the real gap, so
the new `.mvn/maven-build-cache-config.xml` reconciles the surefire skip flags
— any difference in test settings between the cached build and the current run
is now a cache miss, in both directions. I've deliberately not used the
`skipValue` attribute: it *relaxes* the comparison rather than tightening it.
I've also added `-Dmaven.build.cache.enabled=false` to the release publish
step, so the deploy path provably never restores, the same way
`source-build-check.yml` already opts out on purpose.
Worth being explicit about one thing: a restored module skips its tests.
That is the mechanism of the speedup, not a side effect, and it is exactly as
sound as the input list above.
**The problem I found.** The cache stores the actual artifacts. One
generation measures **1.2 GB**, and 1.17 GB of that — 97% — is three bundled
jars:
| module | cache | CI time |
|---|---:|---:|
| xtable-core | 1.0 MB | 17:42 |
| xtable-service | 80 KB | 5:05 |
| xtable-hudi-support-extensions | 11 MB | 34 s |
| **xtable-utilities** | **1.0 GB** | 2:30 |
| **xtable-hive-metastore** | **125 MB** | 28 s |
| **xtable-aws** | **42 MB** | 12 s |
(times from run 31226195551 on main, 27:11 total)
So the three shade modules are 97% of the size and 12% of the time, while
core and service are 84% of the time and about 1 MB. Saving 1.2 GB per merge
into a repository-wide 10 GB budget — currently ~3 GB, almost all
`~/.m2/repository` — would have evicted the very caches it sits beside. Good
catch prompting this.
The fix is the extension's per-project switch,
`<maven.build.cache.enabled>false</maven.build.cache.enabled>`, in those three
POMs; it disables both restore *and* save for the module. Verified locally on
Temurin 11:
- cold build: `BUILD SUCCESS`, cache directory **12 MB** (was 1.2 GB), and
the log confirms `Cache is explicitly disabled on project level` for all three
- warm build: 7 of 7 remaining modules restored, directory still 12 MB
I also bumped the extension 1.1.0 → 1.2.3, which is a correctness bump
rather than housekeeping: MBUILDCACHE-87 (plugin dependencies missing from the
checksum) and MBUILDCACHE-99 (moved files not always detected) both landed in
1.2.1, and both affect whether a restore is *correct* — which matters a lot
more once the cache actually persists. Worth noting the bump also renames the
cache directory from `v1` to `v1.1`, so the first main build after this merges
starts cold by design.
Finally, there's now a guard that skips the save and emits a warning if the
directory ever exceeds 100 MB again, so a future module that starts caching
large artifacts shows up instead of quietly eating the budget. And I raised the
job timeout from 30 to 40 minutes — a cold build is already 27:11 against it,
which is uncomfortably thin regardless of this change.
--
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]
