Drop a run's schemas when the run ends, not when the next one starts - #29
Open
dionmcm wants to merge 1 commit into
Open
Drop a run's schemas when the run ends, not when the next one starts#29dionmcm wants to merge 1 commit into
dionmcm wants to merge 1 commit into
Conversation
Two ordering problems, both of which leak a per-run prospective schema. The drop ran at the START of the next run, so a schema stayed on disk for as long as the instance was idle - all day for a nightly, and forever if the process was replaced, which under Kubernetes it will be. Registration happened only AFTER a successful load, so a load that failed part way through orphaned one permanently. There is no later run that knows the name. Together those held 41 GB in four abandoned schemas on one host, and a later run died with "No space left on device" mid-measurement. The schemas a run creates are now dropped in a finally at the end of that run. Note the nesting in runRF2MysqlValidations: the inner finally wraps the LOADS ONLY and exists to register what they created, including on the failure path; the outer one drops. Getting that wrong deletes the schemas before the assertions query them, which is what the first attempt did. dropRegisteredSchemas is best-effort per schema - one that cannot be dropped logs and does not stop the others - and is still called at the start as a safety net for anything a previous process in THIS instance registered but never reached its own finally for. Previous-release schemas are deliberately still kept unless excludedRF2Files applies: reloading 10 GB per run to save 10 GB of disk is the wrong trade. There is a test pinning that. Six tests. Both halves are load-bearing - reverting the early registration fails all six, and so does reverting the end-of-run drop. NOT fixed here: a schema orphaned by a crash is still unreachable, because schemasToRemove is in-memory. The names embed the run id as epoch millis so an age-based sweep would be safe, but dropping a schema another worker is mid-run on would not be, so that wants its own change.
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
1 participant
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.
A per-run prospective schema outlives the run that created it, by two separate ordering mistakes.
The drop ran at the start of the next run. So a schema stayed on disk for as long as the instance was idle — all day for a nightly — and forever if the process was replaced, which under Kubernetes it will be.
Registration happened only after a successful load. A load that failed part way through orphaned a schema permanently: no later run knows the name.
Together those held 41 GB in four abandoned schemas on one host, and a later run died with
No space left on devicemid-measurement.The change
Schemas a run creates are dropped in a
finallyat the end of that run.The nesting in
runRF2MysqlValidationsmatters and is commented in place: the innerfinallywraps the loads only and exists to register what they created, including on the failure path; the outer one drops, after the assertions have run. Getting that wrong deletes the schemas before the assertions query them — which is what my first attempt did, and what the tests caught.dropRegisteredSchemasis best-effort per schema (one that cannot be dropped logs and does not stop the others) and is still called at the start, as a safety net for anything a previous run in this process registered but never reached its ownfinallyfor.Previous-release schemas are deliberately still kept unless
excludedRF2Filesapplies. Reloading 10 GB per run to save 10 GB of disk is the wrong trade; there is a test pinning that so it does not get "tidied up" later.Tests
Six, and both halves are load-bearing — verified by reverting each on this branch:
Suite on this branch: 219 run, 0 failures, 142 errors, 21 skipped — the same 142 Docker-absent errors as the unmodified base (213 tests + the 6 added here). No Docker available on the machine that ran it.
Not fixed here
A schema orphaned by a crash is still unreachable, because
schemasToRemoveis in-memory. The names embed the run id as epoch millis, so an age-based sweep would be safe, but dropping a schema another worker is mid-run on would not be. That wants its own change and its own discussion.Provenance
Cut from
upstream-developand independent of any other work — no DuckDB, no engine switch, nothing from our fork's other branches.MysqlValidationServiceis untouched by the other PRs in flight, so this merges in any order.