Conversation
mythical-fred
left a comment
There was a problem hiding this comment.
Approved already by two reviewers; I only have nits. The GC logic itself reads correctly, and tolerating duplicate list() reports by clearing indexes rather than removing the entry is a nice touch.
mythical-fred
left a comment
There was a problem hiding this comment.
Nice cleanup — the INFO summary plus first-N sampling of kept/removed paths is exactly what an on-call wants when storage GC misbehaves, and the missing-file ERROR with UUIDs makes silent corruption loud. Test coverage is excellent (cold start, healthy reopen, stray-file disposition, lost-batch detection with a fixed UUID for stable assertions).
A couple of non-blocking notes inline.
|
we should also make sure we surface any errors we encounter during GC when reading those files in the log (if we don't already) |
I'm having trouble understanding this suggestion. |
This makes it clearer what's happening for GC of files at startup: - It logs at INFO level the names of the first several files and directories that it deletes. - It logs the names of unexpected files that it leaves in place. - It logs the counts of all kinds of files that it encounters. In addition, it logs the names of batch files that should appear in storage but do not. It adds tests for these behaviors. Signed-off-by: Ben Pfaff <blp@feldera.com>
|
I pushed an update for everything in the resolved comments. |
sorry I just meant, "in case the gather_files_for_checkpoint_function" has an error, can we make sure we surface it in the log? it may already happen |
Ah. We can do better there, I'll work on it. |
This makes it clearer what's happening for GC of files at startup:
It logs at INFO level the names of the first several files and directories that it deletes.
It logs the names of unexpected files that it leaves in place.
It logs the counts of all kinds of files that it encounters.
In addition, it logs the names of batch files that should appear in storage but do not.
It adds tests for these behaviors.
Describe Manual Test Plan
Added unit tests.
Checklist