chore: deprecate Spark 3.4 rather than removing it in 1.1.0 - #5885
chore: deprecate Spark 3.4 rather than removing it in 1.1.0#5885andygrove wants to merge 3 commits into
Conversation
Spark 3.4 was slated for removal in the 1.1.0 release, but there is still enough usage that removing it now is premature. Keep building and publishing Spark 3.4 binaries, and treat the version as deprecated with no scheduled removal date instead. Drop the Spark 3.4 Spark SQL suite from the merge queue, so it no longer gates a merge. It stays fully runnable on demand: the `run-spark-3.4-tests` label on a pull request, or a manual `workflow_dispatch` of `ci.yml`. Update the docs to match: the removal target, what deprecation now implies for test coverage, and the CI tier tables and diagrams.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed 13ae6e45f653d54735e630026619b2508af12caf.
The change consistently defers Spark 3.4 removal, preserves its Comet test/build and release-publishing paths, and removes only its automatic merge-queue Spark SQL coverage. Other supported Spark versions and the Iceberg queue policy are unchanged.
I found one issue in the new claim that applying the Spark 3.4 label gates the PR. The label-triggered run reports a separate, non-required status, and this change removes the later queue coverage. Details and a suggested correction are inline. No other actionable findings.
Validation:
check-ci-config.pypassed, covering 7 path cases and 16 event-policy cases. Independently executed the base/head routing scripts across unlabeled PR, label-added, labeled synchronization, merge-group, and manual-dispatch scenarios. Only the merge-group Spark 3.4 output changed.- Suite-inventory validation and the remaining Python/Node preflight helpers passed, including all 15 Iceberg shard tests and 4 PR-label tests.
- Current CI run: 57 successful checks and 10 skipped, with no failed or pending checks. The four Spark 3.4 Comet test shards passed. Its separate upstream Spark SQL suite was skipped.
Local validation covered routing scripts and workflow/configuration semantics. I did not launch a hosted failing label run or rebuild/run the native and JVM suites locally for this CI/documentation change.
| # manual workflow_dispatch. It stays in `required_checks.needs` so that a | ||
| # labelled run still gates the pull request it was asked for. |
There was a problem hiding this comment.
[P2] Correct the promise that applying the label gates the PR
Could we correct this guarantee here and in the contributor docs? The documented gh pr edit --add-label run-spark-3.4-tests operation fires a labeled event. On that event, required_checks.name deliberately publishes Required Checks (label run), while .asf.yaml names only Required Checks as required. Keeping spark_3_4 in needs therefore propagates a failure or cancellation to the non-required label status without changing an earlier green required verdict. This PR also removes the subsequent Spark 3.4 queue run, so that later check no longer catches the failure.
I verified the base/head routing scripts and aggregator configuration locally. A subsequent push with the label retained does include Spark 3.4 in the normal Required Checks verdict. Please document that applying the label alone produces an advisory result, and explain the subsequent push needed for the required gate, or implement the gating promised by the current wording.
There was a problem hiding this comment.
Good catch, and you're right — I had the mechanism backwards. Adding the label fires labeled, the aggregator publishes Required Checks (label run) on that event, and .asf.yaml only requires Required Checks, so keeping spark_3_4 in needs does nothing for the label run's own verdict. Dropping the queue run means nothing picks it up afterwards either.
I looked at implementing the gating instead and don't think it's available to us. On labeled the PR tier is deliberately skipped because it already ran at that commit, so publishing that run as Required Checks would overwrite the commit's real verdict with one that tested a single suite — exactly #5007. So I've gone with documenting it.
Driving compute-changes.py for each event shape and pairing it with the name the aggregator publishes:
| Event | spark_3_4 runs |
Aggregator name | Gates the merge |
|---|---|---|---|
| PR opened / pushed, no label | no | Required Checks |
n/a |
run-spark-3.4-tests just added |
yes | Required Checks (label run) |
no, advisory |
| push with the label retained | yes | Required Checks |
yes |
| PR opened with the label | yes | Required Checks |
yes |
merge_group |
no | Required Checks |
n/a |
ac7405d3b replaces the gating promise in the spark_3_4 job comment, the workflows README, contributor-guide/ci.md and contributor-guide/spark-sql-tests.md. Each now says the label run is advisory, that the next push with the label retained is what puts 3.4 into the required verdict, and — since there's no queue run behind it — that the label run's result has to actually be read before merging rather than relied on to block. I added the caveat to spark-sql-tests.md too, since that's where the gh pr edit --add-label command you quoted lives.
Also merged apache/main in. The conflicts were all with #5871, which moved the Spark 4.1 sql_hive shards behind the queue and so edits the same tier listings; resolved by keeping both moves.
sunchao
left a comment
There was a problem hiding this comment.
Completed the follow-up review of 13ae6e45 against base de1eb4f8. The existing P2 about label-run gating remains: applying run-spark-3.4-tests reports the advisory Required Checks (label run) status. A subsequent push with the label retained includes the suite in the normal required verdict. I found no additional P1/P2 issue.
The CI configuration and suite-inventory checks pass locally. An independent base/head comparison across 36 event/path combinations confirms that only Spark 3.4's merge-queue routing changes. These checks validate routing and configuration, not a hosted failing label run.
At 2026-09-12 22:35:20 UTC, CI has 57 successful and 10 skipped checks. Preflight tested merge 8efc606a, whose parents are exactly this base and head, and all nine changed files match the reviewed head. Required Checks passed with Spark 3.4's upstream SQL suite skipped.
…ate-27ab8f26 # Conflicts: # .github/workflows/README.md # .github/workflows/ci.yml # dev/ci/check-ci-config.py
…uest Adding `run-spark-3.4-tests` fires a `labeled` event, and on that event the aggregator deliberately publishes `Required Checks (label run)` rather than the `Required Checks` context `.asf.yaml` requires. A red Spark 3.4 in a label run therefore leaves an earlier green required verdict in place, and with 3.4 no longer in the merge queue there is no later run to catch it either. Spark 3.4 only counts toward the required verdict on a pull_request event that is not `labeled` -- a push with the label already applied. Say so in the `spark_3_4` job comment, the workflows README, the CI guide and the Spark SQL tests guide, rather than promising a gate the label alone does not provide.
ac7405d to
cae1b46
Compare
rich7420
left a comment
There was a problem hiding this comment.
@andygrove thanks for the patch
Which issue does this PR close?
Part of #4329. This PR does not close it — removal is deferred, not cancelled, so #4329 stays open to track the eventual drop. It will need its
1.1.0milestone moved.Rationale for this change
Spark 3.4 was slated for removal in 1.1.0, and the docs said so in three places. We got pushback on that timing, so this PR walks the commitment back: Spark 3.4 becomes deprecated with no scheduled removal date, and we keep building and publishing its binaries.
What deprecation buys us is CI time. The Spark 3.4 Spark SQL suite currently runs in the merge queue on every merge, where it is one of the slowest jobs and a flake in it blocks everyone. Since we are no longer committing to fix Spark 3.4 regressions with the same urgency, it does not need to gate merges.
It does still need to be runnable. Anyone working on
spark/src/main/spark-3.4/ordev/diffs/3.4.3.diffshould be able to get a verdict before merging, andrun-spark-3.4-testsalready exists for exactly that.What changes are included in this PR?
CI.
spark_3_4indev/ci/compute-changes.pygoes from["queue", "label:run-spark-3.4-tests"]to["label:run-spark-3.4-tests"], which drops it from the merge queue while leaving the label andworkflow_dispatchpaths intact:spark_3_4merge_grouprun-spark-3.4-testslabeled+run-spark-3.4-testsworkflow_dispatchNothing else moves tier — 3.5, 4.0, 4.1 and all four Iceberg jobs still run in the queue.
spark_3_4stays inrequired_checks.needs, but @sunchao was right that this does not make alabelled run a gate, and the first version of this PR claimed it did. Adding the label fires a
labeledevent, and on that event the aggregator deliberately publishesRequired Checks (label run)rather than theRequired Checkscontext.asf.yamlrequires, so a red 3.4 there leaves anearlier green required verdict alone. With 3.4 out of the queue there is no later run to catch it
either. 3.4 only counts toward the required verdict on a
pull_requestevent that is notlabeled-- a push with the label already applied, or a PR opened with it:spark_3_4runsRequired Checksrun-spark-3.4-testsjust addedRequired Checks (label run)Required ChecksRequired Checksmerge_groupRequired ChecksMaking the label run itself required is not an option: on
labeledthe PR tier is deliberatelyskipped, so publishing that verdict as
Required Checkswould let it overwrite the commit run andreintroduce #5007. So the fix is to say what actually happens. The
spark_3_4job comment, theworkflows README,
contributor-guide/ci.mdandcontributor-guide/spark-sql-tests.mdnow statethat the label run is advisory, that the next push is what makes 3.4 blocking, and that with no
queue run behind it the label run's result has to actually be read before merging.
Docs.
compatibility/spark-versions.md,installation.md: removal target changes from "the 1.1.0 release" to "a future release", and both now say the Spark SQL suite no longer runs on every change, so a Spark 3.4 regression is more likely to reach a release. The installation CI table readsOn demandfor 3.4's Spark SQL column. The JDK 11 deprecation was sharing a sentence with Spark 3.4 — I split them so its 1.1.0 removal is unaffected.about/versioning_policy.md: the lifecycle said removal happens in "the following" Comet minor release, and gave Spark 3.4 / 1.1.0 as the worked example. Reworded to "a later Comet minor release", removal not scheduled in advance, with "stops gating merges on the Spark SQL suite" folded into what deprecation means. This is the one edit that changes policy rather than just describing the new state — happy to instead keep the next-minor promise and write 3.4 up as a stated exception, if reviewers prefer that.contributor-guide/ci.md,.github/workflows/README.md,ci.ymlheader comments: 3.4 moves out of the queue-tier listings into its own "label or dispatch only" row, with guidance to reach for the label deliberately since nothing else will run it.contributor-guide/spark-sql-tests.md: the "Running Tests in CI" section claimed opening a PR was enough and pointed atspark_sql_test.yaml/spark_sql_test_ansi.yaml, which no longer exist. Rewritten with the per-version reality and thegh pr edit --add-labelcommand.How are these changes tested?
dev/ci/check-ci-config.pyis the test for the routing policy, and it needed updating in a way that is itself the coverage.spark_3_4moves out ofSPARK_OPT_INinto its ownSPARK_DEPRECATEDset, which keeps it inALL_JOBS(soworkflow_dispatchstill asserts it runs) while taking it out ofQUEUE_TIER(so themerge_groupcase now asserts its absence rather than accepting it coming back silently). Added alabeled+run-spark-3.4-testscase to pin that the manual path stays reachable.Locally green:
check-ci-config.py,check-suites.py,test-iceberg-shards.py,apache-rat:check,actionlint --shellcheck=off, andprettier --checkon every file this PR touches.I also drove
compute-changes.pydirectly for each event shape to produce both tables above, pairing each result with the aggregator name that event publishes, rather than trusting the assertions alone.Three markdown files fail
prettier --checkat this branch's base (.ai/skills/review-comet-pr/SKILL.md,contributor-guide/debugging.md,contributor-guide/native_shuffle.md). Pre-existing and untouched here.apache/mainis merged in as off69c4c81b. The conflicts were all with #5871, which moved the Spark 4.1sql_hiveshards behind the queue and so touched the same tier listings this PR edits: theci.ymland README queue-tier prose, the README ASCII diagram and routing table, andSPARK_OPT_IN/ thePOLICY_CASESlist incheck-ci-config.py. Resolved by keeping both changes in each --spark_4_1_hiveadded to the queue tier,spark_3_4taken out of it.