Repository navigation
Conversation
iceberg-spark-extensions ran unsharded and was the longest job in a queue run (55m24s). Shard it exactly like the core iceberg-spark task: the shared shard matrix, --init-script, -PcometShardTask for its own task, and per-shard inventory uploads verified by a new iceberg-spark-extensions-shard-coverage job. dev/ci/check-ci-config.py gains check_iceberg_extensions_shards() so the job cannot silently lose its shard wiring.
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Iceberg extensions tests ran unsharded and were a CI bottleneck.
- Design approach: Reuse the core task’s shard matrix, Gradle predicate, artifact handling, and coverage checker.
- Correctness / compatibility analysis: Found one introduced P1. The predicate reserves shard 1 for core-only streaming tests, leaving extensions without the inventory required by the new gate. Checked upstream extensions sources for all four configured Iceberg/Spark pairs. Spark runtime semantics are unchanged.
- Key design decisions: Shared partition count, independent shard failures, task-qualified artifacts, and coverage verification keep orchestration consistent.
- Implementation sketch: Two files add the extensions matrix, Gradle arguments, uploads, coverage job, and configuration guard.
- Behavioral changes worth calling out: Four extensions workers replace one, but one performs setup/build without executing tests. The projected speedup remains unvalidated.
- Suggested improvements: Use an extensions-appropriate partition and add a regression fixture without streaming tests.
Reviewed the full diff from 9a4d5f28368d4fbb54ac23c14c5d52d5637ce26d to 14199c8658af315ea5fbe9ac177a7ddf03ed50ec. Routed skill: review-comet-pr. No sibling skill applies. The PR is not a draft, and no existing reviews, comments, or threads were present.
Exact-head CI: label passed. Comet CI, Check PR Title, and CodeQL report action_required. Comet CI contains no executed jobs.
Validation: Configuration checks, all 15 shard-checker tests, and workflow actionlint passed. An isolated Gradle 8.14.4/JDK 17 fixture reproduced the coverage failure. Full Spark/Iceberg builds, end-to-end CI, and timing measurements were not run.
| rm -rf /root/.m2/repository/org/apache/parquet # somehow parquet cache requires cleanups | ||
| ENABLE_COMET=true ENABLE_COMET_ONHEAP=true ./gradlew -DsparkVersions=${{ inputs.spark-short }} -DscalaVersion=${{ inputs.scala }} -DflinkVersions= -DkafkaVersions= \ | ||
| :iceberg-spark:iceberg-spark-extensions-${{ inputs.spark-short }}_${{ inputs.scala }}:test \ | ||
| --init-script ../dev/ci/iceberg-test-shards.gradle \ |
There was a problem hiding this comment.
[P1] Use an extensions-specific partition before enabling this init script. CometIcebergTestShard.owner() reserves shard 1 exclusively for TestStructuredStreamingRead* and assigns everything else to shards 2–4. None of the configured extensions source sets contains that family. Consequently, shard 1 becomes NO-SOURCE, Gradle skips the doFirst inventory writer, and the new coverage job fails because inventory 1 is missing, even when every extensions test passes. This makes the Iceberg workflow fail and blocks merge-queue runs that require it. Preserve the core streaming special case while distributing extensions across all four shards, and add a fixture without streaming tests.
Evidence: Checked upstream Iceberg 1.8.1/Spark 3.4, 1.9.1/Spark 3.5, 1.10.0/Spark 3.5, and 1.11.0/Spark 4.1 source trees and Gradle configuration: TestStructuredStreamingRead3 belongs to the core module, not extensions. Reproduced with Gradle 8.14.4/JDK 17 using isolated Java test projects containing passing TestExtendedParser, TestAddFilesProcedure, and TestAlterTableSchema tests and the unchanged repository init script with -PcometShardTask=:test -PcometShardIndex=1..4 -PcometShardCount=4. Baseline executed three tests. Shard 1 reported :test NO-SOURCE and wrote no manifest. Shards 2–4 each passed one test and wrote their inventories. The repository coverage checker exited 1 with Expected shard indices [1, 2, 3, 4], found [2, 3, 4]. Reproduction and logs: /tmp/comet-6113-repro-n141sa7s/run-isolated-repro.py and its isolated/ directory.
There was a problem hiding this comment.
Addressed in 589001e: extensions distribute across all four shards, while the core streaming reservation remains intact. The regression fixtures cover extensions without streaming tests, including small and empty inventories.
Rechecked the current head: all 15 tests in dev/ci/test-iceberg-shards.py and the CI configuration guard passed. The current Comet CI and CodeQL runs are still awaiting approval. Could a maintainer approve them so the hosted jobs can verify this head?
iceberg-spark-extensions ran unsharded and was the longest job in a queue run (55m24s). Shard it exactly like the core iceberg-spark task: the shared shard matrix, --init-script, -PcometShardTask for its own task, and per-shard inventory uploads verified by a new iceberg-spark-extensions-shard-coverage job. dev/ci/check-ci-config.py gains check_iceberg_extensions_shards() so the job cannot silently lose its shard wiring.
Extensions have no structured-streaming test family, so reserving shard 1 leaves it without candidates or an inventory. Hash extensions across all four shards while retaining the core streaming reservation. Exercise both policies with the shared Gradle init script.
The local runner keeps extensions unsharded. Document that exception and provide the direct Gradle recipe for each CI extensions shard.
Keep the verified refreshed tree while retaining the published head as an ancestor, allowing a normal push without rewriting history.
|
Updated against pinned main The shared helper now uses all four hash buckets for the extensions task while preserving the core task's streaming-only reservation. Existing write reports/summaries are retained with shard/attempt-qualified artifact names. Contributor docs clarify that the local extensions target remains unsharded and show how to reproduce a specific CI extensions shard directly. Verification on Apollo:
This was a sequential local verification run, not a measured speedup. Older Iceberg versions, core tests and other runtime matrices were not executed by this verification. Updated head: Current-main diff correctionAll three intended commits were rebased onto Current-base inventory/summary checks, CI guards and all 23 actual Gradle helper-fixture invocations passed with the expected failure controls. Core reservation, four extensions buckets and write reports remain unchanged. The earlier 3,606-test integration result is still older-base evidence; native inputs and the upstream Iceberg patch have changed, so no fresh full-integration result is claimed here. Updated head: |
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Extensions tests ran in one worker while core Iceberg tests already used four shards.
- Design approach: Reuse the existing shard matrix, Gradle helper, artifact handling and coverage checker.
- Correctness / compatibility analysis: No introduced P1/P2 issues found within this review. The earlier empty-shard P1 is addressed. The Gradle fixture verifies that core and extensions shards reproduce their unsharded candidates and executed tests exactly once. Upstream extensions definitions and source sets were checked for all four configured Iceberg/Spark pairs. Spark execution semantics and configurations are unchanged.
- Key design decisions: Extensions use all four hash buckets while core tests retain their streaming reservation. Task-specific coverage jobs and attempt-qualified artifacts preserve rerun handling and write summaries without adding another sharding framework.
- Implementation sketch: The six-file diff updates workflow wiring, task allocation, regression fixtures, CI guards and contributor documentation.
- Behavioral changes worth calling out: Four extensions workers replace one, adding three setup/build executions per configuration. The local extensions target remains unsharded, as documented. Speedup and total compute cost remain unmeasured.
- Suggested improvements: None at the P1/P2 bar.
Reviewed the complete base-relative diff from ca1c143affc051998843e16cc85c3f9c76769bb5 to 589001e3b827fba269d693b0baa96c50e45b4789. The PR remains non-draft. Read the existing review, issue comment, inline comment and thread. Routed skill: review-comet-pr. No sibling skill applies.
Exact-head CI: label passed. Comet CI and CodeQL report action_required. Comet CI has no executed jobs.
Validation: CI configuration checks, 15 inventory tests, six write-summary tests and workflow actionlint passed. The actual Gradle helper fixture completed all 23 invocations with Gradle 8.14.4/JDK 17: 20 successes and three expected failure controls. Full Spark/Iceberg integration, older Gradle versions and performance measurements were not run. The author's older-base integration result was not treated as validation of this head.
Preserve Iceberg extensions sharding alongside upstream suite removals and CI routing changes without rewriting the PR history. Refs apache#6113
|
Merged current main ( Fresh local checks passed: CI configuration, suite inventory, versioned-source parity, benchmark runner (60 suites), Iceberg shard inventory (15 tests), and write-summary checks (6 tests). These are static/configuration checks. The earlier Gradle fixture and 3,606-test integration receipts remain tied to their older base; no fresh full integration result is claimed. The Comet CI and CodeQL runs for this exact head require maintainer approval. Could a maintainer approve them? Please also enable |
|
Updated head: Validated on Apollo against current
Not rerun: the full Iceberg extensions integration; the 3,606-test receipt in the description remains older-base evidence. CI and CodeQL are still |
…0261013 # Conflicts: # dev/ci/check-ci-config.py
|
Merged current The conflict was again limited to Verification on the merged head: GitHub now reports the PR mergeable with the approval retained. Ready for re-review. |
Closes #6103.
Rationale
The
iceberg-spark-extensionsjob ran unsharded and was the longest job in theIceberg workflow — 55m24s in a recent merge-queue run, against four shards for the
core
iceberg-sparktask. It now uses the same shard machinery as the core task.What changes
iceberg-spark-extensionstakes the shared shard matrix(
fromJSON(needs.build-native.outputs.shard-matrix),fail-fast: false), gainsthe
/shard-${{ matrix.shard }}name suffix, and passes the same Gradle wiring asthe core task:
--init-script ../dev/ci/iceberg-test-shards.gradle,-PcometShardTask=:iceberg-spark:iceberg-spark-extensions-...:test,-PcometShardIndex/-PcometShardCount.iceberg-spark-extensions-shard-coveragejob verifies the extensionstask's inventories (
check-iceberg-shards.py --manifests ... --task ...extensions...:test),mirroring the core coverage job.
dev/ci/check-ci-config.pygainscheck_iceberg_extensions_shards()so the jobcannot silently lose its shard wiring.
Expected effect
~55m24s → projected ~15-20 min for the extensions job, roughly 35 queue-minutes
per run. This is the largest single saving identified in the CI review.
How it was tested
python3 dev/ci/check-ci-config.py→CI config checks passed.python3 dev/ci/test-iceberg-shards.py→ 15 tests OK.unshardedCandidatesinventory need one label run to confirm. The new coveragejob fails if the union of shard candidates is not exactly the unsharded
inventory, so a bad split cannot merge silently.
Constraint
SHARD_COUNT = 4indev/ci/check-iceberg-shards.pyis global; the extensionstask reuses the same count.