Conversation
mihaibudiu
left a comment
There was a problem hiding this comment.
I assume this does what it suggests it does
|
Apparently this works too: |
mythical-fred
left a comment
There was a problem hiding this comment.
Approve. Caps cargo/libtest parallelism via CI_RUNNER_CORES (default 20), shards the Java surefire run into A-M / N-Z across both arches with fail-fast: false, scopes the SLT smoke step to one shard, and adds workflow_dispatch so the suite is runnable by hand. Sensible incremental cleanup; the existing cancel-if-tests-java-failed sentinel still triggers from any shard failure.
One real finding plus a couple of nits — all non-blocking.
| - name: A-M | ||
| pattern: "A*,B*,C*,D*,E*,F*,G*,H*,I*,J*,K*,L*,M*" | ||
| - name: N-Z | ||
| pattern: "N*,O*,P*,Q*,R*,S*,T*,U*,V*,W*,X*,Y*,Z*" |
There was a problem hiding this comment.
Shard split is meaningfully lopsided. Counting *Test{s}.java classes under sql-to-dbsp-compiler/SQL-compiler/src/test/java/ at this PR’s tip: A-M = 43 classes, N-Z = 68 — with P* alone contributing 25. Wall time is going to be dominated by the N-Z shard, which defeats most of the point of sharding.
Moving the cut to A-O / P-Z gives 52 / 59 — basically even. Cheap one-character edit (M* → O* in shard 1, N*,O* → nothing in shard 2). Worth doing now while the split is fresh; if you don’t, the next person to add a T*/S* class makes it worse.
|
|
||
| - name: Run mvn test | ||
| run: mvn test --no-transfer-progress -B -pl SQL-compiler -Dsurefire.failIfNoSpecifiedTests=false | ||
| run: mvn test --no-transfer-progress -B -pl SQL-compiler -Dsurefire.failIfNoSpecifiedTests=false -Dtest='${{ matrix.shard.pattern }}' |
There was a problem hiding this comment.
-Dtest= uses surefire glob semantics. Worth confirming locally that letter-class shards interact correctly with the existing fine-grained -Dtest=ClassName#method invocations elsewhere in CI (e.g. RotateTests#quick below — covered here by the A-M scoping, fine). Not a blocker, just a note that if anyone copy-pastes the mvn test ... line they should know about the implicit shard pattern dependency.
| # of the shard pattern). | ||
| - name: Run one quick SLT test | ||
| if: ${{ vars.CI_DRY_RUN != 'true' }} | ||
| if: ${{ vars.CI_DRY_RUN != 'true' && matrix.shard.name == 'A-M' }} |
There was a problem hiding this comment.
Nit: scoping the SLT smoke to matrix.shard.name == 'A-M' is fine, but if the A-M job ever gets if-skipped (e.g. someone adds another if: higher up) the smoke silently never runs. Slightly more robust to make it a separate non-matrix job that needs: the matrix; but for a smoke test this is fine and dropping the duplication is a clear win.
| AWS_ACCESS_KEY_ID: "${{ secrets.CI_K8S_MINIO_ACCESS_KEY_ID }}" | ||
| AWS_SECRET_ACCESS_KEY: "${{ secrets.CI_K8S_MINIO_SECRET_ACCESS_KEY }}" | ||
| # Cap cargo parallelism at the k8s runner core budget so arm64 pods don't | ||
| # OOM from too many parallel linkers. Override via the CI_RUNNER_CORES repo |
There was a problem hiding this comment.
Tiny consistency nit: RUST_TEST_THREADS in test-unit.yml and test-adapters.yml is set at the env level; CARGO_BUILD_JOBS here is also env-level. Good. The fallback || 20 is duplicated five times across workflows — fine for now, but if the answer ever needs to change you’ll touch five files. A repo-level vars.CI_RUNNER_CORES set to 20 would let you drop the || 20 literal everywhere.
mythical-fred
left a comment
There was a problem hiding this comment.
Re-APPROVE on d997598. The OIDC-token-leak fix via split upload-jar job is the right shape, and feldera-minio-cache with AWS_SESSION_TOKEN: "" is good defense-in-depth. A few non-blocking observations on the cache scheme.
| target | ||
| sql-to-dbsp-compiler/temp/target | ||
| sql-to-dbsp-compiler/multi/target | ||
| key: test-java-cargo-${{ steps.cache-month.outputs.month }}-${{ runner.os }}-${{ runner.arch }}-${{ matrix.shard.name }}-${{ hashFiles('**/Cargo.lock') }} |
There was a problem hiding this comment.
The Rust target/ cache is keyed on matrix.shard.name, so A-M and N-Z get two completely separate target/ caches even though both shards compile essentially the same Rust workspace through mvn → sql-to-dbsp. That doubles MinIO storage with little incremental sharing, and the first run of each new month or after a Cargo.lock bump pays the cold-cache cost twice. Dropping ${{ matrix.shard.name }} from this key (keep it in the JVM cache if you want shard-local Maven artifacts) lets the second shard restore from the first one's save. Non-blocking — just leaving money on the table.
| # month so we never restore last month's stale cache. | ||
| - name: Compute monthly cache prefix | ||
| id: cache-month | ||
| run: echo "month=$(date -u +%Y-%m)" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Monthly rotation via date -u +%Y-%m means the first run after a UTC month rollover hits a fully cold cache for everything (both key and restore-keys are prefixed with the new month). On the first of the month at 00:00 UTC, every workflow run pays the full rebuild. Probably fine in practice — but worth noting in case anyone wonders why CI is slow on the 1st. A weekly rotation with +%G-%V would smooth this out without changing the underlying retention story.
| echo "K8S node: ${K8S_NODE_NAME}" | ||
|
|
||
| # Compute the internal-PR boolean once so individual steps can reference | ||
| # it; cheaper to grep and less fragile when triggers expand. |
There was a problem hiding this comment.
Nice consolidation — steps.pr-check.outputs.internal is much clearer than the 3-pronged expression. Tiny nit: in the heredoc, ${{ github.event.pull_request.head.repo.full_name }} is interpolated by Actions, not the shell, so an attacker-controlled fork repo name (e.g. foo" && touch /tmp/pwned && echo ") would land verbatim inside the [[ ... ]]. Fork repo names are validated by GitHub (alphanumerics, hyphens, periods, underscores, slashes), so this is safe today, but using env: indirection and quoting "$HEAD_REPO"/"$ACTOR" is the standard hardening for this pattern and matches the pull_request_target hardening rule already documented in the repo.
mythical-fred
left a comment
There was a problem hiding this comment.
Re-approve on 92dbc11 + 10eb620.
ci: dont cache target/ dirs in java testsdropstarget/,sql-to-dbsp-compiler/temp/target, andsql-to-dbsp-compiler/multi/targetfrom test-java(-nightly) caches and removes the monthly cache prefix scheme. The inline comment makes the tradeoff explicit (20-45 GB temp/target, ~1h gzip, single-use; sccache handles per-crate caching). This subsumes both of my earlier nits: the per-shard 2x storage concern goes away (no shard-keyed target cache), and the first-of-month cold-start goes away (no monthly bucket). Registry cache key now scopes purely onrunner.os/runner.arch/hashFiles('**/Cargo.lock')which is the right granularity for crate downloads.ci: update sccache versionbumps 0.10.0 -> 0.15.0 inbuild.Dockerfile. Confirmed v0.15.0 is the current upstream release. arm64/amd64 URL pattern still resolves; no other Dockerfile changes around it. Worth a smoke test on the build image since sccache 0.10 -> 0.15 spans several releases (cache key format, redis/s3 backend tweaks), but ifsccache --versionworks and a Rust build still emits cache hits in CI, this is fine.
No new findings; APPROVE stands.
mythical-fred
left a comment
There was a problem hiding this comment.
Re-APPROVE on 51c14ec. The new commit cleanly separates concerns:
- Heap profiling is now a cargo feature (
with-heap-profiling) gatingdep:jemalloc_pprof; the/heap_profileendpoint is gated oncfg(all(target_os = "linux", feature = "with-heap-profiling")), and production pipelines still get it via the adaptersdefaultfeature set. The SQL test fixture builds adapters withdefault-features = false, so the non-deterministictikv-jemalloc-sysC build subtree is excluded from the per-test rebuild path. Good targeted fix for the recompile-every-invocation problem. compileAndCheckRustnow setsCARGO_TARGET_DIR=target-checksocargo checkandcargo teststop trashing each others fingerprints..gitignorepicks up**/target-check. Sensible.- The comment on
adaptersOptionsexplains exactly whydefault-features = falseis set for testing only. Production keeps heap profiling. Clean split.
No blockers. Worth noting downstream: anyone building adapters with --no-default-features will now get a runtime 4xx from /heap_profile rather than a Linux-only conditional path. The error string was tightened to heap profiling is not available in this build, which is honest.
mythical-fred
left a comment
There was a problem hiding this comment.
Re-APPROVE on fa68cabe3d. Per commit message this is an explicit experiment to compare wall-clock against sccache.
The change is clean as a controlled A/B:
- Drops
RUSTC_WRAPPER=sccache+SCCACHE_CACHE_SIZE/SCCACHE_BUCKET+CARGO_INCREMENTAL=0+ the trailingsccache --show-statsstep. - Keeps
SCCACHE_ENDPOINT/SCCACHE_REGION+ AWS creds becausefeldera-minio-cachereuses them as its S3 config (correctly called out in the comment). - Re-introduces full
target/(+sql-to-dbsp-compiler/temp/target+multi/target) into the cached paths, keyed onmatrix.shard.nameso A-M and N-Z don't trash each other's target dirs. - Monthly rotation prefix matches the existing pattern.
Two non-blocking nits for when you read the experiment results:
- The original removal rationale said
target/ballooned to 20-45 GB per shard and that gzipping it cost ~1h of wall clock. If save-time is similar this round, total wall-clock = sccache_savings - target_save_cost. Worth grabbing the actual save-step durations from the run log, not just the test duration, before declaring a winner. - Cache key includes
hashFiles('**/Cargo.lock')but not the Rust toolchain version — arust-toolchain.tomlbump would still restore staletarget/artifacts (cargo mostly rebuilds, but the ABI mismatch window is real). Probably fine for an experiment; flag it if this shape sticks.
Approve stands.
mythical-fred
left a comment
There was a problem hiding this comment.
Re-APPROVE on 3c1982c (head fa68cab -> 3c1982c). Two new commits on top of the prior re-APPROVE:
1d28eb7 adds the zstd apt package to deploy/build.Dockerfile so runs-on/cache (and thus feldera-minio-cache) auto-uses zstd instead of falling back to gzip. Image bumped to sha-1d28eb7711... across every workflow file that pins it.
3c1982c sets ZSTD_CLEVEL=1 + ZSTD_NBTHREADS=${CI_RUNNER_CORES||20} workflow-env-wide for test-java, and splits the java shard matrix from 2-way A-M/N-Z into 3-way A-I/J-Q/R-Z. The -Dtest SLT smoke selector is correctly re-pointed at A-I. The comment explicitly calls out that R-Z stays slightly heavier because R* (Regression*) and P* (Postgres*) are indivisible by class-name pattern -- exactly the right caveat to leave in tree, and a much better outcome than the original ~43/68 imbalance I flagged on the first pass.
Mechanical wins, no functional regressions visible. APPROVE stands.
Two non-blocking observations:
ZSTD_CLEVEL: 1favors speed over ratio, which is the right call on a 100Gb LAN with multi-GB target/ archives. Worth a glance at the next CI sample to confirm the wall-clock save onSave cacheactually drops; ifZSTD_NBTHREADSends up oversubscribing alongsideCARGO_BUILD_JOBS=20during the save step (cache save runs after the build, so likely fine), the symptom would be a slower save than the per-arch baseline.- The 3-way split doubles the matrix dimension; you now have 6 java-test jobs per workflow run (2 archs x 3 shards). Make sure the runner pool has headroom -- this is fine on the current k8s pool but is worth keeping in mind before adding a 4th shard.
mythical-fred
left a comment
There was a problem hiding this comment.
Re-APPROVE on 22b6993.
Single new commit reverses course: the 3-way A-I/J-Q/R-Z matrix collapses back to a single all shard per arch, with the balanced split left commented immediately below as documentation. With zstd 0.15-tier restore times this is the right call — paying the per-shard build.sh + restore cost N times only makes sense if surefire wall-time dominates, and the prior shard runs apparently showed otherwise. The 3-way config is kept verbatim so re-enabling sharding is a one-edit operation; the matrix dimension stays so re-sharding doesn't require touching needs:/if: selectors. The SLT smoke gate moves to matrix.shard.name == 'all' cleanly. Nothing to flag.
mythical-fred
left a comment
There was a problem hiding this comment.
Re-APPROVE on 891a42b473.
Single 3-line change in .github/workflows/test-java.yml: drops the experimental comments and replaces the cache-key hashFiles('sql-to-dbsp-compiler/calcite_version.env', 'sql-to-dbsp-compiler/**/pom.xml') with an explicit list of the four real source poms (sql-to-dbsp-compiler/pom.xml, SQL-compiler/pom.xml, simulator/pom.xml, slt/pom.xml). The inline comment explains why: the **/pom.xml glob walks the restored target/ trees (millions of files) and blows past hashFiles' 120s timeout. Correct diagnosis, correct fix, accurate comment. No new findings.
mythical-fred
left a comment
There was a problem hiding this comment.
Re-APPROVE on 891a42b473.
Single commit replaces **/pom.xml with an explicit list of the four source poms in the test-java-jvm cache key. The motivation is sound: with target/ now restored before the cache step, **/pom.xml would walk millions of files under target trees and routinely blow past hashFiles' 120s budget. Explicit listing is the right escape hatch.
One non-blocking note: the JVM cache key still glob-hashes 'sql-to-dbsp-compiler/**/pom.xml' over in build-java.yml; that job doesn't restore target/, so the timeout isn't a risk there today — but if/when build-java also gains a target cache, the same explicit-list treatment will be needed.
mythical-fred
left a comment
There was a problem hiding this comment.
Re-APPROVE on 891a42b473. Drops the EXPERIMENT preamble (the cache-target/ vs sccache experiment was concluded by the prior commit, so the comment was just lingering) and, more importantly, replaces hashFiles('sql-to-dbsp-compiler/**/pom.xml') with an explicit four-file list. Good catch: once target/ is in the restored cache, the glob has to stat millions of files and blows past the documented 120s hashFiles limit — a perfectly silent failure mode where the cache key would just go empty and you'd get a cold start. The four-file enumeration is right for the current module layout; if a new pom.xml gets added the cache key won't reflect it and Maven will misbehave until the list is updated, but that's the right trade for not making the cache step time bomb.
APPROVE stands.
Add the feldera-minio-cache composite action (runs-on/cache against the gha-cache bucket) and migrate the cargo registry and JVM caches in the rust, java, and pre-mergequeue workflows onto it. Swatinem/rust-cache is dropped: it cannot target a custom S3 backend. Cap cargo and libtest parallelism at the runner core budget via the CI_RUNNER_CORES variable so arm64 pods stop OOMing on parallel linkers. Split the JAR S3 upload into its own job so the OIDC session token never reaches the cache-save phase, which MinIO rejects. Add zstd and bump sccache to v0.15 in the dev image: runs-on/cache uses zstd for cache compression when present, else falls back to slow gzip. Java tests cache the cargo registry and JVM deps only, not target/: caching the multi-GB target/ trees never paid off. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Make jemalloc_pprof an optional dependency enabled by a new with-heap-profiling feature (on by default). The SQL test fixtures build adapters with default-features = false, so they drop the jemalloc / tikv-jemalloc-sys C-build subtree, whose non-deterministic build fingerprint forced a recompile on every test invocation. Production pipelines keep default features and retain the /heap_profile endpoint. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Truncate the generated udf.rs and restore empty stubs after each UDF test via a shared cleanupUdf helper, so a stale udf.rs never leaks into a later test. Give 'cargo check' its own target-check directory: check and test carry different per-profile fingerprints, and sharing one target dir re-dirties the dependency graph on every switch, so the build never settles. Drop the adapters default features in the generated test crates to exclude jemalloc_pprof. Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Regenerated from the build.Dockerfile change (zstd CLI + sccache v0.15). Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
mythical-fred
left a comment
There was a problem hiding this comment.
Re-approving (#11). The four new commits are all surgical:
feldera-minio-cachecomposite action: nice consolidation. SettingAWS_SESSION_TOKEN: ""to defend against a stray OIDC token poisoning the cache save phase is a thoughtful touch, and splittingupload-jaroff into its own job (with its ownid-token: write) so the OIDC session never enters the build environment is the right architectural fix rather than papering over it.CARGO_INCREMENTAL=0+CARGO_BUILD_JOBS=${{ vars.CI_RUNNER_CORES || 20 }}: correct interactions with sccache (no incremental) and the arm64 OOM-on-parallel-linkers issue. Threading the same var into--test-threadsfor the doc-test step keeps it consistent.with-heap-profilingfeature gate: makingjemalloc_pprofoptional + cfg-gating/heap_profileso the SQL test fixture stops dragging in the non-deterministictikv-jemalloc-sysC build — exactly the right shape. Keeping it indefaultpreserves production behavior.cleanupUdf+ separatetarget-checkdirectory: the "check and test fingerprints diverge" diagnosis is well-stated in the commit body and the fix is local and reversible.
Two small notes, take or leave:
download-artifactgot pinned back fromv8tov7across the touched workflows. If that's a deliberate rollback for a known issue, a one-liner comment somewhere would save future archaeology.- The
pr-checkstep usesif: github.event_name == 'pull_request'for the internal-PR boolean but never gates the step itself by event — forpush/merge_groupruns theifshort-circuits tofalseandinternal=falseis emitted, which is what later steps want, so this is correct, just non-obvious. Worth a comment.
Neither blocks.
No description provided.