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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
