andygrove commented on issue #5838: URL: https://github.com/apache/datafusion-comet/issues/5838#issuecomment-5623183097
I looked into how we'd actually turn this on. Short version: it's self-serve in `.asf.yaml` now, no INFRA ticket needed. `.asf.yaml` rulesets accept a raw GitHub Rulesets API payload, and `asfyaml/feature/github/rulesets.py` passes raw payloads straight through with no rule-type allowlist — its `_NUMERICAL_VALUE_KEYS` set even names the merge-queue parameters explicitly, so it's deliberate rather than a loophole. apache/iceberg-python, apache/seatunnel and apache/texera are all running merge queues configured this way; `gh api repos/apache/iceberg-python/rulesets` shows the live ruleset. seatunnel is the closest match to our setup because it keeps the new `rulesets` block alongside classic `protected_branches` on the same branch, which is what we need since our review requirement lives there. Worth saying that apache/datafusion is not the template here, even though it's the obvious place to look. Their `.asf.yaml` has no merge-queue config at all and `gh api repos/apache/datafusion/rulesets` returns `[]` — INFRA turned theirs on directly in classic branch protection via INFRA-27154 in September 2025, before asfyaml supported rulesets. apache/datafusion#6880 has the full history. What's worth copying from them is the workflow side, and more usefully the mistake: apache/datafusion#17538 and its revert in apache/datafusion#17629 are them enabling required checks, discovering that a workflow which never triggers leaves its required check pending forever, and backing it out. We have a nastier version of that same problem. Our heavy jobs in `ci.yml` are thin callers of reusable workflows, and the check name a caller publishes depends on whether it ran: - skipped by `if:` — one check run named exactly `PR Build (Linux)`, conclusion `skipped` - actually ran — only `PR Build (Linux) / Spark 4.1, JDK 17 [exec]` and friends, and no bare `PR Build (Linux)` at all I confirmed both against real commits (bdd2aeb and 36caf8e). So there is no name that reports in both cases: requiring the bare name hangs every code PR, requiring a nested name hangs every docs-only PR, and in both directions it hangs rather than fails. A required context that never reports also blocks the merge that would fix `.asf.yaml`, and only INFRA can remove a required check by hand at that point. The way out is a single flat aggregator job — one `Required Checks` that `needs:` everything and goes red only on `failure`/`cancelled` — and that being the only required context. On the split itself, what I've got working: - PR tier: preflight, changes, build Linux/macOS, benchmark, Spark 4.1, Iceberg 1.11 - Queue tier: all of the above plus Spark 3.4/3.5/4.0 and Iceberg 1.8/1.9/1.10 - push to main: docs deploy only That last line matters more than it looks. If we add a queue but keep the existing full push-to-main runs, every merge pays for CI twice and total load goes up rather than down. The saving comes from PRs no longer running Spark 3.5 on every push, plus the queue replacing the push-to-main run instead of adding to it. Spark 4.1 stays in the PR tier because it's the default build profile and so the cheapest early warning that something is wrong. Two things that need doing outside the repo. First, `run-spark-3.4-tests` doesn't exist as a label (only `run-spark-4.0-tests`, `run-iceberg-tests` and `run-spark-4.2-tests` do), so that escape hatch has been quietly doing nothing already; we'd need to create it and `run-spark-3.5-tests`. Second, this should merge in two steps — the `ci.yml` changes first so we can watch `Required Checks` report correctly on real PRs, and only then the `.asf.yaml` change that makes it required and turns the queue on. Getting that order wrong is precisely the case that needs an INFRA ticket to recover from. One thing worth raising more widely before we flip it: once these suites gate the queue, a flaky test blocks everyone's merges rather than just one PR. datafusion sent a dev@ note ahead of enabling theirs, and given this changes how every committer merges, we should probably do the same. -- 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]
