feat(amber): wait out Lakekeeper's asynchronous purge when deleting a warehouse - #7744
Open
mengw15 wants to merge 3 commits into
Open
feat(amber): wait out Lakekeeper's asynchronous purge when deleting a warehouse#7744mengw15 wants to merge 3 commits into
mengw15 wants to merge 3 commits into
Conversation
Contributor
Backport auto-label reportThis
|
Contributor
Automated Reviewer SuggestionsBased on the
|
… warehouse deleteWarehouseEmptyFirst drops every table with purgeRequested=true, then immediately deletes the warehouse entity. Lakekeeper purges the dropped tables' data files asynchronously (task queue `tabular_purge`) and refuses to delete the warehouse while any purge is pending -- the tasks need the warehouse's storage profile to reach S3, so deleting it first would orphan them and leak the files. It answers 409 WarehouseHasUnfinishedTasks until the queue drains, so the first delete of any warehouse that had ever stored execution results always failed; a retry seconds later succeeded. Retry the final warehouse delete on exactly that conflict -- 409 with error.type WarehouseHasUnfinishedTasks -- with a bounded pause (default 10 retries x 2s; the queue normally drains within seconds). Every other error, including any other 409, still fails immediately, and 404 stays the idempotent goal state. The bound and delay are constructor parameters with defaults, so production call sites are unchanged and the spec injects a zero delay -- no real sleeps in the tests. LakekeeperClientSpec covers the three outcomes against its in-process stub: 409-409-204 succeeds with exactly 3 attempts (failed before the fix on the first 409), a never-draining queue fails after the bounded 1+3 attempts, and a 409 of any other type fails on the first attempt with no retry. Found while testing the flag-gated per-user warehouse feature; no deployment is affected because the flag defaults to off. Closes apache#7742.
mengw15
changed the title
mengw15
force-pushed
the
fix/7742-warehouse-delete-purge-retry
branch
from
August 18, 2026 00:31
e82ece1 to
6ce6cba
Compare
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 367 | 0.224 | 26,328/38,549/38,549 us | 🟢 -43.0% / 🔴 +141.1% |
| 🔴 | bs=100 sw=10 sl=64 | 824 | 0.503 | 119,393/147,794/147,794 us | 🔴 +11.3% / 🔴 +37.7% |
| ⚪ | bs=1000 sw=10 sl=64 | 947 | 0.578 | 1,053,370/1,111,522/1,111,522 us | ⚪ within ±5% / 🔴 -9.7% |
Baseline details
Latest main 60300e3 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 367 tuples/sec | 345 tuples/sec | 779.07 tuples/sec | +6.4% | -52.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.224 MB/s | 0.21 MB/s | 0.476 MB/s | +6.7% | -52.9% |
| bs=10 sw=10 sl=64 | p50 | 26,328 us | 24,289 us | 12,818 us | +8.4% | +105.4% |
| bs=10 sw=10 sl=64 | p95 | 38,549 us | 67,670 us | 15,986 us | -43.0% | +141.1% |
| bs=10 sw=10 sl=64 | p99 | 38,549 us | 67,670 us | 19,339 us | -43.0% | +99.3% |
| bs=100 sw=10 sl=64 | throughput | 824 tuples/sec | 864 tuples/sec | 1,011 tuples/sec | -4.6% | -18.5% |
| bs=100 sw=10 sl=64 | MB/s | 0.503 MB/s | 0.527 MB/s | 0.617 MB/s | -4.6% | -18.5% |
| bs=100 sw=10 sl=64 | p50 | 119,393 us | 114,934 us | 100,965 us | +3.9% | +18.3% |
| bs=100 sw=10 sl=64 | p95 | 147,794 us | 132,833 us | 107,295 us | +11.3% | +37.7% |
| bs=100 sw=10 sl=64 | p99 | 147,794 us | 132,833 us | 115,531 us | +11.3% | +27.9% |
| bs=1000 sw=10 sl=64 | throughput | 947 tuples/sec | 951 tuples/sec | 1,049 tuples/sec | -0.4% | -9.7% |
| bs=1000 sw=10 sl=64 | MB/s | 0.578 MB/s | 0.581 MB/s | 0.64 MB/s | -0.5% | -9.7% |
| bs=1000 sw=10 sl=64 | p50 | 1,053,370 us | 1,053,787 us | 978,248 us | -0.0% | +7.7% |
| bs=1000 sw=10 sl=64 | p95 | 1,111,522 us | 1,158,992 us | 1,021,881 us | -4.1% | +8.8% |
| bs=1000 sw=10 sl=64 | p99 | 1,111,522 us | 1,158,992 us | 1,050,075 us | -4.1% | +5.9% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,545.08,200,128000,367,0.224,26328.33,38548.94,38548.94
1,100,10,64,20,2427.06,2000,1280000,824,0.503,119392.83,147793.59,147793.59
2,1000,10,64,20,21123.08,20000,12800000,947,0.578,1053369.73,1111522.46,1111522.46
Contributor
There was a problem hiding this comment.
Pull request overview
Adds bounded retry handling for Lakekeeper’s asynchronous warehouse purge conflicts.
Changes:
- Retries only
409 WarehouseHasUnfinishedTasks. - Adds deterministic success, exhaustion, and unrelated-conflict tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
LakekeeperClient.scala |
Implements bounded warehouse-delete retries. |
LakekeeperClientSpec.scala |
Tests retry and failure behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7744 +/- ##
============================================
+ Coverage 91.02% 91.03% +0.01%
- Complexity 4454 4466 +12
============================================
Files 1174 1174
Lines 47146 47163 +17
Branches 5287 5292 +5
============================================
+ Hits 42916 42937 +21
+ Misses 2550 2549 -1
+ Partials 1680 1677 -3
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The retry only fires for a 409 whose body carries the WarehouseHasUnfinishedTasks type, and the type check parses that body -- so a 409 with a non-JSON body (a gateway error page, say) must read as 'not the purge conflict' and fail immediately rather than be waited out as if it were transient. Assert it fails on the first attempt.
…ueue The wait was a fixed 2s x 10. That made the common case -- a purge that drains almost immediately -- cost the caller a full 2s before the delete was retried, on a request a user is waiting on, while still issuing 11 requests when the queue is genuinely slow. Double the pause instead, from 200ms up to a 5s cap over 7 retries: 0.2+0.4+0.8+1.6+3.2+5+5s, so a fast purge returns in ~200ms, a slow one issues fewer requests, and the overall bound drops from 20s to ~16s. Tests still inject a zero initial delay -- doubling zero stays zero, so they contain no real sleeps.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
LakekeeperClient.deleteWarehouseEmptyFirstempties the warehouse by dropping every table withpurgeRequested=true, then immediately deletes the warehouse entity. Lakekeeper purges the dropped tables' data files asynchronously (task queuetabular_purge) and refuses to delete the warehouse while any purge is pending — the tasks need the warehouse's storage profile to reach S3, so deleting it first would orphan them and leak the files. It answers409 WarehouseHasUnfinishedTasksuntil the queue drains, so in practice the first delete of any warehouse that has ever stored execution results always failed; a retry seconds later succeeded (details in #7742).409witherror.type == "WarehouseHasUnfinishedTasks"is treated as transient and retried with exponential backoff — 200ms doubling up to a 5s cap over 7 retries (0.2+0.4+0.8+1.6+3.2+5+5s ≈ 16s bound). A purge that drains almost immediately, the common case, costs the caller ~200ms rather than a full fixed interval; a genuinely slow one issues fewer requests than a fixed pause would. Every other error — including any other 409, and a 409 whose body isn't the expected JSON envelope — still fails immediately, and 404 stays the idempotent goal state.Found while testing the flag-gated per-user warehouse feature (#6870); no deployment is affected because the flag defaults to off.
Any related issues, documentation, discussions?
Closes #7742. Part of #6870;
deleteWarehouseEmptyFirstintroduced in #7473. Verified against the local Lakekeeper (0.11.0) that its management API exposes no task-query endpoint or force-delete option, so waiting out the documented conflict type is the only client-side path.How was this PR tested?
LakekeeperClientSpec(in-process HTTP stub, no external infra) gains four cases mirroring the issue's repro: the stub answers the warehouse delete with409 WarehouseHasUnfinishedTaskstwice and then 204 — verified failing before the fix (the first 409 threw) and passing after, with the stub asserting exactly 3 delete attempts; a warehouse whose queue never drains fails after the bounded 1+3 attempts; a 409 of any other type fails on the first attempt with no retry; and a 409 whose body is not JSON (a gateway error page) also fails on the first attempt rather than being waited out.WorkflowExecutionService/scalafmtCheck(main + Test) passes.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-4-8)