comphead commented on PR #5843:
URL:
https://github.com/apache/datafusion-comet/pull/5843#issuecomment-5640612473
I checked the ASF side of this against the asfyaml source and GitHub's merge
queue docs, and read the workflows at `6cacd83`. The claims that would sink it
all hold up:
- `asfyaml/feature/github/rulesets.py` forwards a raw `rules` list to GitHub
untouched. The only rule-type inspection is the `copilot_code_review` overlap
check, so `merge_queue` needs no INFRA ticket.
- It coerces exactly the merge-queue numeric keys plus `actor_id`
(INFRA-27864). It *silently leaves* a non-integer alone, so a typo surfaces as
a GitHub 422 rather than an asfyaml error.
- `apache/root` is team id `118420`. The repo currently has zero rulesets,
so `reconcile_rulesets` creates `Merge Queue` cleanly.
- `merge_method: SQUASH` matches `enabled_merge_buttons`.
- `apache/iceberg-python` is real prior art, tuned much more conservatively:
`max_entries_to_build: 1`, `max_entries_to_merge: 1`,
`check_response_timeout_minutes: 90`, no bypass actors.
## Rollout risk
**Rehearse `merge_group` on a scratch branch first.** The path has never
executed. If `changes` errors on a merge group under `set -euo pipefail`,
`required_checks` goes red, every entry is evicted, and the fix has to merge
through the broken queue. This is self-serve: land a ruleset targeting a
literal `refs/heads/mq-rehearsal` with both `merge_queue` and
`required_status_checks: Required Checks`. Merge-queue rules reject wildcards,
not literal refs, and `ci.yml` has no `branches:` filter on `pull_request` or
`merge_group`, so a throwaway PR against that branch exercises the whole path.
A ruleset on a scratch branch cannot wedge `main`.
**Make the merge_group diff fail open**, as cheap insurance:
```bash
git diff --name-only "$MQ_BASE_SHA"..."$MQ_HEAD_SHA" > changed_files.txt || {
echo "::warning::merge_group diff failed; running everything"
git ls-tree -r --name-only "$MQ_HEAD_SHA" > changed_files.txt
}
```
**Add a project-side bypass actor.** `apache/root` still means an INFRA
ticket. `apache/datafusion-committers` exists (id `9960695`) and would let the
PMC merge the fix directly. Ruleset bypass does not bypass classic branch
protection, so the one-approval requirement still applies.
## Gaps in the diff
**The `actions/cache` exception was applied to Linux only.** `build_linux`
was kept on `push` for cache freshness, but it is not the only job writing
caches nothing else writes:
| Cache key | Written at | Only writer | Push tier after this PR |
| --- | --- | --- | --- |
| `macOS-cargo-ci-v2-*` | `pr_build_macos.yml:89` | `build_macos` | gone |
| `macOS-cargo-registry-*` | `pr_build_macos.yml:280` | `build_macos` | gone
|
| `macOS-java-maven-*` | `actions/java-test/action.yaml:70` | `build_macos`
| gone |
| `Linux-spark-sql-*` | `actions/setup-spark-builder/action.yaml:55` |
`spark_sql_test_reusable.yml` | gone from all four callers |
| `Linux-benchmark-maven-*` | `pr_benchmark_check.yml:63` | `benchmark` |
gone |
The explicit saves are guarded on `github.ref == 'refs/heads/main'`, false
on a `gh-readonly-queue/*` branch, and the `actions/cache@vN` auto-saves land
in a queue scope deleted with the branch. `restore-keys` means gradual
degradation rather than a cliff, but `Linux-spark-sql-*` is the
`~/.m2/repository` for the suites now on the critical path of every merge, and
a cold macOS cargo build on every queue run works against
`max_entries_to_build`. Options: accept it and say so in `POLICY`, add `"push"`
back to `spark_4_1` on the same grounds, or add a push-only cache-warm macOS
job without the test matrix.
**In the queue, a fork PR's code runs with a write-capable `GITHUB_TOKEN`.**
A `merge_group` run is on a base-repo branch, so it gets the repo default token
permissions and repository secrets, and `actions/checkout` persists that token
into `.git/config`. Under `pull_request` those same suites get a read-only
token and no secrets. `docs.yaml` pushes to `asf-site` with no `permissions:`
block, which only works if the repo default is read and write, and neither
`ci.yml` nor any reusable workflow it calls declares one. Cargo build scripts,
Maven/sbt plugins and the applied Spark diff all run in that context. Fix is
small, and also clears the CodeQL finding you hit on `required_checks`:
```yaml
permissions:
contents: read
jobs:
docs:
permissions:
contents: write # pushes to asf-site
```
**Four workflows sit outside the umbrella and outside the queue.**
`codeql.yml`, `delta_build_gate.yml` and `pyarrow_udf_test.yml` trigger on
`pull_request` and `push: main` with no `merge_group`. They are never evaluated
against the merge result, are not in `required_checks.needs`, and are not
required, so a red one does not stop a queueing. They also still run on `push`,
which contradicts the no-push-tier rule stated two files away. Either fold them
into `ci.yml` as reusables so they inherit the aggregator, or document them as
advisory. `check-ci-config.py` only reasons about jobs inside `ci.yml`, so a
new top-level workflow escapes the gate silently.
## Tuning
**`check_response_timeout_minutes: 300` is calibrated on the PR tier.** The
queue tier adds three Spark SQL suites (each with its own ~40 minute build
job), three Iceberg suites, macOS and the benchmark check. At
`max_entries_to_build: 2` that is roughly 150 concurrent jobs from this repo
against a shared ASF pool, so wall clock is set by scheduling delay rather than
fan-out. You can measure it without the queue: apply all six opt-in labels and
then push a commit, since on `synchronize` the gates read from `PR_LABELS` and
one run reproduces the queue tier exactly. Not `workflow_dispatch` though,
since `event_allows` returns `True` unconditionally for it including for
`docs`, and `docs.yaml` pushes to `asf-site` with no ref guard.
**The cost model turns on the rebuild rate, not the tier sizes.** Per merged
PR with `P` CI-triggering pushes and `B` merge-group builds charged to it:
```
Δ = P × (macOS + benchmark + Spark 3.5) − (B − 1) × FULL − Linux-on-push
```
A full pipeline is several times `macOS + benchmark + Spark 3.5`, so every
extra build charged to an entry eats several pushes' worth of savings. Probably
still net negative at a typical `P`, but the margin is governed by `B`, so
tracking evictions per week from day one is what decides whether this worked.
**`ALLGREEN` is the right default, and there is a stronger argument for it
than the comment gives.** Under `HEADGREEN` with `SQUASH`, a group of two lands
two commits on `main` and the first was never green on its own, which breaks
bisect. The counterweight is that GitHub explicitly recommends `HEADGREEN` for
flaky CI, and this repo has enough infra flakiness to justify retry wrappers on
both upload and download. Worth naming the flip condition in the `.asf.yaml`
comment, for example an eviction rate threshold over a week. One-line change
either way.
## Minor
- `allow_update_branch: true` becomes mostly redundant once the queue is
live, and each click costs a full PR-tier rerun.
- Do not add `strict: true` later. Requiring branches to be up to date
fights the queue.
- `check-ci-config.py` could assert `merge_method` matches the single
enabled entry in `enabled_merge_buttons`, since drift there breaks every merge.
- The `build_linux` comment says queue caches "are deleted with the branch".
For the three explicit `cache/save` steps the queue never writes them at all,
because of the `refs/heads/main` guard. Same outcome, different mechanism.
- Agree on the dev@ note. That is also where the eviction-and-requeue
recovery path should be spelled out for contributors.
--
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]
