Conversation
|
@gz This is a followup to your suggestion to make sure we're logging storage errors. |
The storage interface `StorageBackend::list` did not distinguish between errors that meant that some or all of the files in a directory were not listed and errors that meant that an individual file's metadata could not be obtained; it reported them all the same way, through its return value. This commit changes that. Now, errors that could lose files are still reported through the return value, and errors that mean that the metadata for an individual file cannot be obtained are reported through the callback. This allows the caller to more intelligently interpret the errors. This commit also improves error logging: the POSIX backend now logs warnings for errors obtaining metadata (up to a limit), and the checkpointer code logs the number of directory entry errors encountered during GC. Signed-off-by: Ben Pfaff <blp@feldera.com>
mythical-fred
left a comment
There was a problem hiding this comment.
Clean separation between "could-lose-files" errors (return value) and "per-entry metadata" errors (callback) — the API shape is the right one and the GC log now actually surfaces the count. Trait doc on list is good. Three things worth addressing before this lands, plus two nits inline.
Test gap (the main thing). The new behavior — per-entry file_type: Err(_), the rate-limited warn ladder, and the loop continuing after a readdir entry error to return the last one — has no test. The existing gc_startup test only exercises the all-Ok happy path. Suggest a thin wrapper around MemoryBackend (similar to the PosixWrapper you already added in the checkpointer test module) that lets a test inject Err(_) for chosen entries, then assert: (a) n_errors ≥ 1 appears in the GC log, (b) Class::Other fallback path is taken, (c) on entries with Err(NotFound) cb is not called.
Posix readdir-entry errors are now returned, not skipped. Previously a readdir entry Err set result = Err(e) and continued the loop — same as today — but the loop now returns the last such error rather than the first. Worth a sentence in the trait doc ("returns the last one") which you already added — good — but please also note that errors are accumulated (cb continues for subsequent OK entries) so callers can't rely on Err meaning "listing stopped". The current doc reads correctly to me; flagging only because the changelog/commit message doesn't mention this.
Posix NotFound race for metadata is silently swallowed. That matches prior behavior (the status.json.mut → status.json rename case), but now a metadata NotFound results in neither a warn nor a callback nor a bump to any counter. Fine for the documented race, but if a real file vanishes between readdir and stat you'll never know. Consider counting NotFound-skipped entries separately and including the count in the same GC log line — it's free and would catch unexpected churn.
| _ => debug!("I/O error listing {parent}: {e}"), | ||
| } | ||
| } | ||
| cb(entry); |
There was a problem hiding this comment.
Style nit, non-blocking: this checks &entry.file_type twice (once for the NotFound short-circuit, once for the warn). Reads cleaner as a single match:
match &entry.file_type {
Err(e) if e.kind() == ErrorKind::NotFound => continue,
Err(e) => match warnings.next().unwrap() { /* … */ },
Ok(_) => {}
}
cb(entry);| let entries = path.read_dir().map_err(|e| { | ||
| StorageError::stdio(e.kind(), "readdir", self.fs_path(parent).display()) | ||
| })?; | ||
| let mut warnings = 0usize..; |
There was a problem hiding this comment.
Minor: using 0usize.. as a counter so you can do warnings.next().unwrap() is a bit cute — the unwrap is technically unreachable (you'd need usize::MAX entries first) but it reads as fallible. A plain let mut n_warnings = 0usize; then n_warnings += 1; match n_warnings { 1..20 => warn!(…), 20 => warn!(…bridge…), _ => debug!(…) } says the same thing without the never-fires .unwrap(). Holzmann would prefer no unwrap on a hot path.
| .list(&StoragePath::default(), &mut |path, file_type| { | ||
| .list(&StoragePath::default(), &mut |DirEntry { name, file_type}| { | ||
| let file_type = file_type.unwrap_or_else(|_| { | ||
| n_errors += 1; |
There was a problem hiding this comment.
An Err(_) here collapses any per-entry metadata failure into Class::Other + Disposition::KeepUnexpected. That's reasonable — refuse to delete what you can't classify — but the GC log line will show those entries as Keeping unexpected Other … even though the real reason is "could not stat". Worth a one-line debug log at the point of the unwrap_or_else (debug!("Cannot determine type of {name}: {e}, treating as Other")) so operators can correlate the n_errors count with concrete paths.
| })?; | ||
| info!( | ||
| "GC kept {}/{}/{} expected files/directories/other, kept {}/{}/{} unexpected, and deleted {}/{}/{} unused", | ||
| "GC kept {}/{}/{} expected files/directories/other, kept {}/{}/{} unexpected, and deleted {}/{}/{} unused; {n_errors} error(s) reading directory entries", |
There was a problem hiding this comment.
Nit: the GC log line is now wide enough that I'd flip it to fielded logging (info!(expected_files = …, n_errors, "GC summary")) so log-aggregators can index on n_errors and alert on > 0. Format-string version stringifies a number that operators will want to filter on.
The storage interface
StorageBackend::listdid not distinguish between errors that meant that some or all of the files in a directory were not listed and errors that meant that an individual file's metadata could not be obtained; it reported them all the same way, through its return value.This commit changes that. Now, errors that could lose files are still reported through the return value, and errors that mean that the metadata for an individual file cannot be obtained are reported through the callback. This allows the caller to more intelligently interpret the errors.
This commit also improves error logging: the POSIX backend now logs warnings for errors obtaining metadata (up to a limit), and the checkpointer code logs the number of directory entry errors encountered during GC.
Describe Manual Test Plan
Just ran the unit tests.
Checklist