feat(catalog-rest): add REST server-side scan planning client - #3011
feat(catalog-rest): add REST server-side scan planning client#3011daviddallakyan2005 wants to merge 3 commits into
Conversation
Port plan / fetch-result / cancel / fetch-tasks onto RestCatalog with a WaitForPlan poller. Task decoding and TableScan routing stay follow-ups, so supports_remote_scan_planning remains false. Part of apache#1690.
A huge Retry-After used to panic Duration::from_secs. Ignore overflow, await cancel when the poller times out, and DELETE the plan if the wait future is dropped.
RFC 9110 allows both delta-seconds and IMF-fixdate. Ignoring the date form made wait_for_plan retry immediately against a server that asked us to wait. Tests also cover cancelled/failed/expired poll outcomes and the namespace 404 split.
| (min_delay, max_delay, grace, max_retries, clamp_retry_after) | ||
| } | ||
|
|
||
| fn next_scan_plan_backoff(prev: Duration, min_delay: Duration, max_delay: Duration) -> Duration { |
There was a problem hiding this comment.
Consider reusing backon instead of the hand-rolled backoff. backon is already a workspace dependency (backon = "1.5.1" in the root Cargo.toml) and is already the repo pattern for retry loops — see crates/iceberg/src/transaction/mod.rs:206 (ExponentialBuilder::new().with_min_delay(..).with_max_delay(..).with_max_times(..)).
The realistic scope: this is a poll-until-terminal-state loop, not a plain retry, so backon won't model the Retry-After handling, the submitted → completed/failed/cancelled state machine, or the drop-guard cancel, keep those. But the generic exponential-jitter part (next_scan_plan_backoff + resolve_wait_options delay math) could be ExponentialBuilder, which would also let us drop the new rand dependency added to this crate. WDYT?
| } | ||
|
|
||
| /// True when a fetch-result 404 was a forgotten plan-id. | ||
| pub fn is_plan_expired(err: &Error) -> bool { |
There was a problem hiding this comment.
These public predicates work by string-matching err.message() against the MSG_* constants (and is_plan_failed even does starts_with(format!("{MSG}: "))). That's fragile as a public API — a message tweak silently breaks callers, and matching on error text is unusual for this codebase. Consider a typed error/kind (or a dedicated planning-error enum) so callers can match structurally. Not blocking, but it's the part of the public surface I'd most want to firm up before it's relied on.
There was a problem hiding this comment.
In fact do we need this at all in the public api? Seems like we only use this in tests?
There was a problem hiding this comment.
Same question for the other helpers below
xanderbailey
left a comment
xanderbailey
left a comment
There was a problem hiding this comment.
Nice work! Excited to see server-side planning work happen! Left a few comments.
2 participants
Which issue does this PR close?
What changes are included in this PR?
Working REST client for server-side scan planning (Go's
catalog/rest/scan_planning.go), without wiringTableScan. Design note: #1690 (comment)DEFAULT_ENDPOINTS.RestCatalog(plan_table_scan,fetch_planning_result,cancel_planning,fetch_scan_tasks,wait_for_plan).Catalogis untouched.error.type(NoSuchTable/NoSuchNamespace/NoSuchPlanId/NoSuchPlanTask).Idempotency-Keyon POSTs; opaqueplan-idas a single path segment.wait_for_planpoller with jittered backoff, retry on 408/429/5xx, cancel on timeout / max retries / drop.Retry-Afteraccepts RFC 9110 delta-seconds and IMF-fixdate (HTTP-date). Overflowing second counts are ignored instead of panicking.supports_remote_scan_planningstays false until task decoding exists (same as Go today). File payloads keepdata-fileas JSON.Follow-ups: content-file decoder,
TableScanrouting, plan-scoped FileIO (#2651/#2932), DataFusion (#2671).wait_for_planis a per-plan poll loop (default 10 retries after the first GET, jittered 100ms–5s backoff, one GET plus JSON parse per attempt) and completed/fetch-tasks payloads keep each data-file and delete-file as aserde_json::Valuetree, so a large plan materializes the full REST JSON DOM in memory once rather than decodedFileScanTaskstructs.Are these changes tested?
Mockito unit tests in
iceberg-catalog-rest(no docker /iceberg-rest-fixture):cargo test -p iceberg-catalog-rest --lib cargo clippy -p iceberg-catalog-rest --all-targets --all-features -- -D warnings cargo fmt -p iceberg-catalog-rest -- --check cargo public-api -p iceberg-catalog-rest --all-features -ssLocal result: 109 lib tests passed, clippy
-D warningsclean,public-api.txtupdated. GitHub CI on the previous commits (including workspaceTests (default)) was green; this follow-up adds HTTP-dateRetry-Afterplus cancelled/failed/expired/namespace-404 coverage.AI Disclosure
https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions
AI assistance was used to draft the client, tests, and this description. The port was checked against Go
scan_planning.goand existing REST catalog patterns. Tests and clippy were run locally as above.Reviewer focus:
wait_for_plandrop-cancel clones an uninitializedRestCatalog(an extraGET /v1/configon abort) and spawns the DELETE rather than awaiting it, so a Tokio worker Drop cannot deadlock.