Don't log NotFound as a critical error in delete_evicted_splits - #6622
Open
joepio wants to merge 1 commit into
Open
Don't log NotFound as a critical error in delete_evicted_splits#6622joepio wants to merge 1 commit into
joepio wants to merge 1 commit into
Conversation
remove_file() failures here were logged as "critical... disk space leak" unconditionally, including on plain NotFound, and the actual io::Error was discarded so a real failure and "someone already removed it" were indistinguishable. The sibling .temp cleanup a few lines above already special-cases NotFound; this mirrors that and surfaces the real error for the cases that remain.
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.
Description
SearchSplitCache::delete_evicted_splitslogs everyremove_filefailure as a "critical" error implying an uncounted disk-space leak, but it doesn't special-caseio::ErrorKind::NotFoundand discards the actualio::Error, so a genuine failure and "someone already removed it" are indistinguishable from the logs.The sibling
.tempcleanup a few lines above in the same file already handles this correctly:This PR mirrors that pattern for
delete_evicted_splits, and includes the realio_errin the log for the cases that remain.Why this matters in practice
On a self-hosted single-node deployment (0.8.1) with a very short
commit_timeout_secs(aggressive backfill ingest, lots of small-split churn), we run an external sidecar that also removes orphaned split-cache files directly (diffing the cache directory against the metastore'sPublishedset) as a workaround for cache-pollution buildup. When that sidecar wins the race againstdelete_evicted_splitsfor the same file, Quickwit's own eviction logs the "critical... disk space leak" message above even though the outcome (file gone) is exactly what was wanted. There's no way to tell that apart from an actual leak without attaching a debugger, since the realio::ErrorKindis discarded.Even without an external deleter in the picture, any two callers racing eviction for the same split (e.g. overlapping LRU decisions) would hit the same false alarm.
How was this PR tested?
Small, mechanical change mirroring an existing pattern in the same file; no behavior change for genuine (non-NotFound) failures beyond now logging the real error.