nevzheng commented on PR #12241: URL: https://github.com/apache/gravitino/pull/12241#issuecomment-5227989055
Hi all. Thanks @laserninja, @lasdf1234, and @roryqi. I have reviewed the design and discussion and would like to share my review, verdict, and proposed next steps for this effort. **Overall:** LGTM on Phase-1 design direction. **Defer (blocking follow-up):** Phase-2 ticket/design (ranges §8.5 + caching from [#12254](https://github.com/apache/gravitino/issues/12254)). ### Next steps **Phase 1 design: LGTM** (see understanding + scoping below). **P0 — Deliver Phase 1** 1. **@laserninja** — proceed with Phase-1 implementation (#12194). **P1 — Close this design PR / park Phase 2** 2. **@lasdf1234 @roryqi** — close out open threads on this PR; migrate any remaining topics or open discussions into the Phase-2 ticket below. If we still need @yuqi1129, please track him down and ask — fine if he’s busy; we just need this thread closed. 3. **@laserninja** — open a Phase-2 follow-up ticket / design (ranges §8.5, caching starting from [#12254](https://github.com/apache/gravitino/issues/12254), related cost work). **Defer (blocking follow-up)** into that track — not a gate on this design PR. I’m happy to help review and give input. **P2 — Clarity (non-blocking)** 4. **@laserninja** — **suggestion (strong, non-blocking):** add sequence diagrams for the `/plan` → `plan-task` → `/tasks` flow, and explicitly list which CUJs Phase 1 vs Phase 2 cover. **P3 — Impl review** 5. **@lasdf1234** — smaller impl-detail questions (e.g. `PlanTaskCodec` nesting) → hash out in #12194 implementation review, not on this design doc. ## Scope — Phase 1 ### IRC spec — scan planning compliance **Understanding** - Complete Iceberg REST Catalog scan planning: `POST .../plan` + `POST .../tasks`, opaque `plan-task`, advertise `/tasks` in `/v1/config`. - We’re following the IRC spec for server-side planning, so there aren’t many API-shape decisions to make here. **Verdict:** LGTM - **note:** Wire protocol is Iceberg’s; our decisions are mostly behind the opaque `plan-task`, not the API shape. ### State model **Understanding** - Stateless: compute on every `/plan` and `/tasks` call. - Continuation lives in the opaque `plan-task` (no plan DB / shared plan-id session). - Caching is optional for *correctness* (not the source of truth). **Verdict:** LGTM · **Defer (blocking follow-up)** — caching (#12254, Phase-2 track) - **issue (non-blocking):** Stateless recompute without a real caching design is an operational hazard (staleness, multi-replica, invalidation, load amplification). Does **not** block merging this design doc. - **suggestion (non-blocking):** @laserninja @lasdf1234 @roryqi — **Defer (blocking follow-up)** into the Phase-2 track: own a caching design/process from [#12254](https://github.com/apache/gravitino/issues/12254). Do not treat “turn the cache on” as a small toggle. I’m happy to help review. ### Metadata / continuation model (`plan-task`) **Understanding** - Opaque `plan-task` = replay params (table + pinned scan/snapshot + offset/limit). - No stored task list; `/tasks` recomputes and slices. - Encoding can change later without an IRC API break. **Verdict:** LGTM w/ non-blocking comments · **Defer (blocking follow-up)** — caching (#12254, Phase-2 track) - **note:** This interaction model is fairly opaque; sequence diagrams help a lot. I trust the owners to implement it. - **issue (non-blocking):** Recompute-heavy redeem + weak caching ⇒ perf / cost / operational risk (self-DoS-shaped load under concurrent `/tasks`). Does **not** block merging this design doc. - **suggestion (non-blocking):** Mitigate in layers: (1) **Defer (blocking follow-up)** caching/#12254 on the Phase-2 track (@laserninja @lasdf1234 @roryqi); (2) **Defer** Phase 2 §8.5 manifest-range plan-tasks (recompute less); (3) **Defer** later shared cache / coalesce as needed. ### Correctness bar **Understanding** Under Phase-1 recompute, paging is only safe if all of the following hold (design §5.2, §5.5.4, §5.6, §5.11; pin + total order also stressed in Rory’s thread): 1. **Snapshot pinning** — `plan-task` embeds the planned snapshot so later commits don’t move the scan. 2. **Total order** — tasks are totally ordered before offset/limit so independent recomputes slice the same way (incl. dup-path fix). 3. **Authz on POST** — `POST .../plan` and `POST .../tasks` authorize from the **URL table**; plan-task is **not a capability**. 4. **MoR deletes (Phase 1)** — full planner + per-response `delete-files` for that page’s tasks (no cross-page delete indexes). **Verdict:** LGTM - **praise:** Snapshot pinning and total order — the right correctness foundation for stateless redeem; good catch on the sort key in review. - **praise:** Path-based authz + not-a-capability is solid. - **note:** Phase-1 MoR path looks correct; the harder delete-index problem belongs to Phase 2 ranges (§8.5), not this bar. ### Config **Understanding** - Principle: **good defaults + configurability**, with a clear mechanism to change behavior. - Phase-1 paging knob: `gravitino.iceberg-rest.scan-plan-task-batch-size` - **default `100`** — page size for file-scan-tasks (~0.3MB vs common ~1MB LB caps; Iceberg-aligned) - **`0`** — escape hatch back to all-inline `/plan` - other values — operators can tune page size - Batching is always applied (not gated on cache). Cache on/off is separate (#12254). - Mechanism: standard Gravitino Iceberg REST config property (documented; not a hidden constant). **Verdict:** LGTM w/ follow-up requested - **note:** Confirming **good defaults + configurability**: default `100` looks right; `0` and tunable values are the right escape/tune mechanism via `scan-plan-task-batch-size`. - **suggestion (non-blocking):** Follow-up — revisit the default under real workloads if 100 causes too many `/tasks` RPCs (§8.2); keep the property as the single knob (don’t add parallel undocumented ways to set page size). Best, Nevin PS — I will write up thoughts on Phase 2. -- 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]
