You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Solaris-star (via GitHub)" <gi...@apache.org> on 2026/07/30 08:55:00 UTC
[PR] fix(rest): release fetched FileScanTask state to prevent unbounded memory growth [iceberg]
Solaris-star opened a new pull request, #17429:
URL: https://github.com/apache/iceberg/pull/17429
## Summary
Fixes #17427
The reference REST server-side scan planning implementation retains all planned `FileScanTask` objects in the singleton `InMemoryPlanningState` even after clients have successfully fetched every plan task. Neither `fileScanTasksForPlanTask` nor `nextPlanTask` removes the fetched entries, and the state is only released via `cancelPlan`. Per the REST Catalog OpenAPI spec, cancellation is not required after all plan tasks have been fetched, so a successful fetch lifecycle must release state without an explicit cancel request.
## Fix
Add two methods to `InMemoryPlanningState`:
- **`releasePlanTask(planTaskKey)`** — removes a single fetched plan task's `FileScanTask` list and next-task link
- **`releaseAsyncPlanForTask(planTaskKey)`** — removes the async planning state when the last plan task in a chain is fetched (extracts the planId from the key format `{planId}-{tableId}-{sequence}`)
Call both from `CatalogHandlers.fetchScanTasks` after building the response:
```java
IN_MEMORY_PLANNING_STATE.releasePlanTask(planTask);
if (nextPlanTasks.isEmpty()) {
IN_MEMORY_PLANNING_STATE.releaseAsyncPlanForTask(planTask);
}
```
## Tests
Added `TestInMemoryPlanningState` with 4 unit tests:
- `releasePlanTask` removes both file scan tasks and next-task link
- `releaseAsyncPlanForTask` removes async planning state
- malformed keys are handled gracefully (no exception, no side effects)
- releasing unknown keys is a no-op
**Note:** I could not run the full Gradle test suite locally (partial clone conflicts with the multi-module build), but the changes are minimal `Map.remove()` calls following existing patterns in the codebase. CI should validate.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): release fetched FileScanTask state to prevent unbounded memory growth [iceberg]
Posted by "Solaris-star (via GitHub)" <gi...@apache.org>.
Solaris-star closed pull request #17429: fix(rest): release fetched FileScanTask state to prevent unbounded memory growth
URL: https://github.com/apache/iceberg/pull/17429
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): release fetched FileScanTask state to prevent unbounded memory growth [iceberg]
Posted by "vbhanuchander-lang (via GitHub)" <gi...@apache.org>.
vbhanuchander-lang commented on PR #17429:
URL: https://github.com/apache/iceberg/pull/17429#issuecomment-5287768873
@Solaris-star you closed this yourself, but for what it's worth I verified the bug and the spec
basis independently before I found your PR, and I think this was worth landing rather than
withdrawing.
**The leak is real.** In `InMemoryPlanningState`, `fileScanTasksForPlanTask` (line 112) and
`nextPlanTask` (line 121) are plain `get` calls; the only removals are in `cancelPlan`
(lines 165-166) and the full `clear()`. The state is a process-lifetime singleton, so it grows for
every successful scan.
**The spec supports releasing on fetch**, which is the part that turns this from "a cache with no
eviction" into a defect. `rest-catalog-open-api.yaml` says, of `cancelPlanning`:
> Cancellation is not necessary after `fetchScanTasks` has been used to fetch scan tasks for each
> plan task.
and again, that cancellation is unnecessary when "Scan tasks for each plan task have been fetched
using fetchScanTasks". A spec-conformant client that fetches everything therefore never cancels, so
if the server only frees state on cancel it leaks on exactly the happy path. That matches the
reporter's 25M retained tasks and >20GB heap over 22 hours in #17427.
If you reopen it, two things a reviewer would likely raise — neither of which looks fatal to me:
1. **Fetch becomes non-idempotent.** After `releasePlanTask`, a client retrying a `fetchScanTasks`
that it never received the response for gets `NoSuchPlanTaskException` instead of the tasks. The
spec does not promise repeat fetches, and `NoSuchPlanTaskError` is a defined response, so this is
defensible — but it is a behaviour change worth stating explicitly in the PR description, and it
is the obvious argument for TTL-based eviction as an alternative.
2. **`releaseAsyncPlanForTask` parses the planId out of the key** by counting hyphens backwards. That
works for the current `{planId}-{tableId}-{sequence}` format but silently does nothing if the key
shape changes, and returns early on malformed keys. Threading the planId through explicitly, or
keeping a task→plan map, would be sturdier.
I have deliberately not opened a competing PR — this is your work and it was first. If you would
rather not pick it back up, say so and I am happy to take it forward with credit to you; otherwise
reopening and @-mentioning a reviewer is probably all it needs. The same thing happened on #17070
this week: the author's PR was closed for inactivity, was revived after someone pointed at it, and
merged within hours.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org