Repository navigation
[#11196] docs(design-docs): Add Iceberg rewrite manifests job design - #12942
Conversation
Documents the design of builtin-iceberg-rewrite-manifests: why manifest bloat is a distinct problem from data file bloat, why the Spark SQL procedure was chosen over the Iceberg Java action, the parameter surface and its validation, the unresolved-placeholder hazard that shapes the job's input handling, and the metrics-then-policy sequencing needed before the job can be triggered automatically. Requested during review of apache#12937. Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
Code Coverage Report
|
| |----------|-------------|-------------------------| | ||
| | Hourly batch load | 24 | ~720 | | ||
| | Streaming, 1-minute checkpoints | 1,440 | ~43,200 | | ||
|
|
There was a problem hiding this comment.
Could u align your table?
| | `builtin-iceberg-update-stats` | Writes the statistics and metrics policies read | No, submitted directly or by the CLI | | ||
| | `builtin-iceberg-rewrite-data-files` | Merges small **data** files | Yes, `system_iceberg_compaction` | | ||
| | `builtin-iceberg-expire-snapshots` | Drops old snapshots and their metadata | No, submitted directly | | ||
|
|
There was a problem hiding this comment.
Could u align your table?
| | Approach | Pros | Cons | Decision | | ||
| |----------|------|------|----------| | ||
| | A. Engine-side delegation only (status quo, #10500) | Already merged; zero new server code | Needs a human in a Trino session; not policy-drivable; useless to non-Trino deployments | Rejected - leaves the server-side gap that #11196 is about | | ||
| | B. Iceberg Java `RewriteManifests` action, called directly | Type-safe; exposes `rewriteIf`, `stagingLocation`, `specId`; no SQL text to escape | Requires catalog-loading plumbing no other job has; binds us to `iceberg-core` internals across the `compileOnly` boundary; diverges from all three sibling jobs | Rejected - see below | |
There was a problem hiding this comment.
Could u align your table?
| | Main class | `org.apache.gravitino.maintenance.jobs.iceberg.IcebergRewriteManifestsJob` | | ||
| | Registered in | `BuiltInJobTemplateProvider.BUILT_IN_JOBS` | | ||
| | Spark configs | `IcebergSparkConfigUtils.buildTemplateSparkConfigs()`, shared with every Iceberg job template | | ||
|
|
There was a problem hiding this comment.
Could u align your table?
| | `use_caching` | `--use-caching` | boolean | No | Iceberg's default (`true`) | Cache table metadata in Spark for the duration of the rewrite | | ||
| | `spec_id` | `--spec-id` | int | No | The table's current spec | Rewrite manifests to this partition spec ID | | ||
| | `spark_conf` | `--spark-conf` | JSON object | No | None | Extra Spark configuration applied to the session | | ||
|
|
There was a problem hiding this comment.
Could u align your table?
| |-------|----------|------|---------| | ||
| | Catalog name | `escapeSqlIdentifier` | Double internal backticks, wrap in backticks | ``catalog`; DROP TABLE t; --`` becomes ``` `catalog``; DROP TABLE t; --` ``` | | ||
| | Table identifier | `escapeSqlString` | Double single quotes | `db.t' OR '1'='1` becomes `db.t'' OR ''1''=''1` | | ||
|
|
There was a problem hiding this comment.
Could u align your table?
| | `use_caching: "false"` | `--use-caching false` | `Boolean.parseBoolean` gives `false` | Correct | | ||
| | *nothing* | `--use-caching {{use_caching}}` | `Boolean.parseBoolean` gives `false` | **Wrong** - silently disables caching instead of leaving Iceberg's default | | ||
| | *nothing* | `--spec-id {{spec_id}}` | `Integer.parseInt` throws | Job fails, though the caller asked for nothing | | ||
|
|
There was a problem hiding this comment.
Could u align your table?
| |-----------|----------|----------|---------------------| | ||
| | `use_caching` | `true`, `false`, any case | Anything else, for example `yes` | `Boolean.parseBoolean` maps every non-`true` string to `false`, so a typo would quietly invert the caller's intent | | ||
| | `spec_id` | Non-negative integer | Negative, non-numeric | Keeps a malformed value from reaching Spark as an unparseable SQL literal, and reports it against the argument name rather than as a SQL syntax error | | ||
|
|
There was a problem hiding this comment.
Could u align your table?
| | `custom-avg-manifest-size` | `AVG(length)` | Distinguishes many-small from few-large | | ||
| | `custom-small-manifest-number` | `COUNT(*)` below a threshold | Directly comparable to `custom-small-file-number` | | ||
| | `custom-manifest-spec-count` | `COUNT(DISTINCT partition_spec_id)` | More than one live spec is what makes `spec_id` useful | | ||
|
|
There was a problem hiding this comment.
Could u align your table?
|
The spec_id parameter is documented as optional with default “the table’s current spec”, but operators still have no way to know which integer to pass (0 vs 1 vs …). That is too abstract for anyone who evolved the table from day partitioning to hour partitioning. Could we add a short “How to find spec_id” note in the Parameters section (and ideally in the user-facing CLI docs that #12937 owns)? Something like: Omit spec_id unless you need to rewrite against a non-default spec. Omission uses Iceberg’s current spec and only consolidates small manifests. Pass the id of the spec you want new manifests written with (usually the current one, e.g. hour after a day→hour evolution). Also worth one sentence of clarification: spec_id does not filter which manifests are rewritten. The procedure still rewrites all manifests of the current snapshot; spec_id only chooses the partition spec used for the new manifests. |
| `use_caching` and `spec_id` never reach the statement as caller text. They are validated, then re-rendered from the parsed `boolean` and `int`, so the only characters that can appear are those `Boolean.toString` and `Integer.toString` produce. | ||
|
|
||
| ### Placeholder resolution, and why absent parameters need care | ||
|
|
There was a problem hiding this comment.
This section mainly focuses on common JobManager issues. In fact, it has little relation to manifests file rewriting. Could you please remove this section?
|
This document seems to have a slightly unusual structure. Could it be made to have a similar structure to #11700? |
| | `custom-avg-manifest-size` | `AVG(length)` | Distinguishes many-small from few-large | | ||
| | `custom-small-manifest-number` | `COUNT(*)` below a threshold | Directly comparable to `custom-small-file-number` | | ||
| | `custom-manifest-spec-count` | `COUNT(DISTINCT partition_spec_id)` | More than one live spec is what makes `spec_id` useful | | ||
|
|
There was a problem hiding this comment.
custom-manifest-number and custom-avg-manifest-size indicators, I have conducted research and found examples in the industry. Are there any precedents in the industry for the other two? If not, we can start by collecting only the first two indicators.
Align the design with the orphan-files proposal, simplify metrics, and document verified spec selection and discovery. Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
|
Updated in 02e0463:
One correction to both my earlier description and the suggested wording: in Iceberg 1.11.0, I also corrected the caching default: the 1.11.0 action defaults to false, so the docs now defer to the installed runtime instead of promising true. Validation: license audit, docs build, formatting check, Markdown table alignment, and |
I have no other questions regarding this document. Please add these points to it. I will review let Rory review this design document. |
Define trigger defaults, per-spec object statistics, consistent spec selection, and the initial Spark/Iceberg compatibility target. Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
|
Added all three points:
Updated the PR plan to cover threshold boundaries, map updates, and default-spec changes between collection and submission. The former open questions are now recorded as resolved decisions. These are design changes; policy/collector implementation remains in the follow-up PR plan. |
| | `use_caching` | `--use-caching` | Boolean | No | Installed Iceberg version's default | | ||
| | `spec_id` | `--spec-id` | Non-negative integer | No | Table's current spec ID | | ||
| | `spark_conf` | `--spark-conf` | JSON object | No | No additional overrides | | ||
|
|
There was a problem hiding this comment.
According to the latest regulations, jobConf key and CLI argument should be same. You can see this PR:#13134
There was a problem hiding this comment.
Updated the design and implementation in #12937. The jobConf keys now match CLI flags without the leading --: catalog, table, use-caching, spec-id, and spark-conf. Updated templates, API examples, CLI docs, and tests together. Spark catalog configuration also uses {{catalog}} for this job; existing jobs retain their default placeholder. Iceberg SQL still uses use_caching and spec_id. All 31 targeted job tests, formatting checks, and the design license audit passed. Original co-author credit is preserved.
|
@laserninja There is a little issue. Try to fix it and this design is OK for me. @roryqi Could u take a look?Thank you. |
Yes all issue i mentioned that you have fixed. |
Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
|
@laserninja |
Keep the built-in job naming style after the CLI/key equality restriction was withdrawn in review. Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
|
Thanks for clarifying. Restored the original built-in job naming style in the design (1644226) and implementation #12937 (c4ea3f0):
Reverted the naming-only template, test, example, and catalog-helper changes together. The earlier spec-selection guidance, per-spec statistics design, trigger thresholds, and runtime support decisions are preserved. Updated both PR descriptions as well. |
|
@roryqi This design doc is good for me. Could u take a look?Thank you. |
…13329) ### What changes were proposed in this pull request? Implements the first phase of the merged design in #12942: the `builtin-iceberg-rewrite-manifests` Spark job, registration, validated arguments, spec selection, and operator documentation. Manifest statistics and the Policy -> Strategy -> Adapter integration for automatic triggering remain separate follow-ups in the design, as requested in review. Continues #12937 on current main and preserves the original commits by @ibrahimErbilen from #11216, with co-author attribution on follow-up work. Adds Spark-backed tests invoking the job entry point through resolved template arguments. They verify default and explicit partition-spec selection, unchanged table records and other-spec manifests, repeat-run no-op behavior, and cleanup without metadata changes after an unknown-spec failure. The entry point checks the Iceberg runtime, closes Spark on success or failure, prints user-visible results to stdout for output.log, and reports concise errors to stderr with a nonzero exit. Strict argument parsing is shared through IcebergJobUtils while preserving existing callers. The overview and configuration guides document direct submission and the remaining policy integration. ### Why are the changes needed? Manifest rewriting maintains manifest layout independently of data-file compaction. The previous implementation lacked Spark-backed verification of partition-spec selection and failure cleanup. Fix: #11196 ### Does this PR introduce _any_ user-facing change? Adds `builtin-iceberg-rewrite-manifests` with required `catalog_name` and `table_identifier`, plus optional `use_caching`, `spec_id`, and `spark_conf`. Existing jobConf/CLI naming conventions are retained. Invalid, duplicate, or missing argument values fail before session creation; omitted optional values retain Iceberg defaults. `spec_id` selects an existing spec, without migrating files between specs. ### How was this patch tested? - 183 jobs-module tests passed, including 33 manifest unit tests and 3 Spark-backed tests on Spark 3.5 / Iceberg 1.11.0. - `./gradlew :maintenance:jobs:test :maintenance:jobs:spotlessCheck :maintenance:jobs:javadoc :docs:build rat -PskipITs` passed with JDK 17. - Process-level checks verified exit code 1, concise stderr, and no stdout or stack trace for missing arguments and malformed Spark configuration. - `git diff --check` passed. Spark tests use a local Hadoop catalog; remote catalog deployment and the server REST submission path were not exercised end to end. --------- Co-authored-by: İbrahim ERBİLEN <ibrahimerblnn46@gmail.com>
…ter table cycles (#13717) ### What changes were proposed in this pull request? Rebase the collector directly onto current main and remove the dependency on #13532. Collect every spec from one pinned table snapshot with one `allManifests` traversal. Publish both complete maps and `custom-manifest-snapshot-id` in one existing PUT/update call. Default to last-write-wins publication without a coordination directory. Consumers requiring fresh statistics must compare the published snapshot ID with the table's current snapshot and skip/recollect on mismatch. An optional shared coordination directory retains per-table locking through publication and explicit recovery after ambiguous writes. Collection failures are always rerunnable. The collector forces `gravitino.client.retryTimes=0`; other clients retain the default of one retry. ### Why are the changes needed? Follow the single-writer direction agreed in #13532. Avoid scanning the same manifest list once per spec and introducing a public merge API solely for this collector. Related to #11196 and design #12942. ### Does this PR introduce _any_ user-facing change? Adds `builtin-iceberg-update-manifest-stats` with optional `coordination_directory`. Omitted or blank uses last-write-wins; opting in requires a shared filesystem supporting durable files and cross-host Java/POSIX locks. Collection is table-wide with no `spec_id` option. Defined specs without manifests publish zeroes; each update replaces the complete maps, removing stale entries. Snapshot ID is -1 when no snapshot exists. Automatic policy triggering remains separate. ### How was this patch tested? - 222 jobs tests, 19 optimizer API tests, and 30 focused Java client tests passed. - Coverage includes partition evolution, empty tables, one snapshot/manifest pass, independent processes contending for the lock, uncoordinated publication and reruns, safe reruns after collection failures, ambiguous PUT recovery, auth alias propagation, and HTTP retry counts of zero, one, and two. - Formatting, jobs Javadocs, `:docs:build`, `rat`, and `git diff --check` passed. - Cross-host filesystem failure behavior was not exercised; deployments must verify shared lock/durability semantics.
3 participants
What changes were proposed in this pull request?
Design for
builtin-iceberg-rewrite-manifests, following the numbered architecture and layer structure of #11700. Covers template registration, procedure parameters, spec ID discovery, execution, safety, and the implementation plan.The initial job is submitted directly. Follow-up statistics are limited to
custom-manifest-number-by-specandcustom-avg-manifest-size-by-spec; automatic policy triggering remains separate implementation work. The design defines defaults of 500 manifests (critical), or 100 manifests with average size below 8 MiB, and preserves one resolved spec ID across collection, evaluation, and submission. Initial CI/runtime support is Spark 3.5 with Iceberg 1.11.0.Related: #11196, #12937 (implementation), #8864. Continues the original work by @ibrahimErbilen in #11216, with co-authorship retained.
Why are the changes needed?
Operators need a dedicated manifest maintenance job and clear guidance after partition evolution. The document explains that
spec_idselects manifests belonging to an existing spec; it does not migrate data files between specs. Includes the Iceberg 1.11.0 source supporting that behavior.Does this PR introduce any user-facing change?
No runtime changes. User-facing documentation is maintained in #12937.
How was this patch tested?
git diff --check../gradlew ratpassed; companion CLI documentation passed./gradlew :docs:build.