Fix delete obsolete files on recovery not rate limited #12590
Conversation
jowlyzhang
force-pushed
the
discard_stall_on_open
branch
from
826a65c
to
ada13ef
Compare
db/db_impl/db_impl.h
Outdated
| // A set of all the directories used by this DB. | ||
| // Note that these paths do not have a tailing path separator "/" | ||
| // (kFilePathSeparator). | ||
| std::unordered_set<std::string> all_db_paths_; |
There was a problem hiding this comment.
My sense is that it would be better to use an ordered set here, for predictability of behavior. For example I believe there are some cases where the error returned on a multiply-corrupt DB depends on the iteration order of this set. It's less confusing if that error is not dependent on factors like how RocksDB was compiled. This could also appear as flaky test behavior in the future.
Data structure performance should not really be a concern here, as these data structure ops are generally paired with IO ops.
There was a problem hiding this comment.
Thanks for catching this change that may cause regression.
db/db_impl/db_impl_files.cc
Outdated
| mutex_.AssertHeld(); | ||
| std::vector<std::string> paths; | ||
| paths.push_back(NormalizePath(dbname_ + std::string(1, kFilePathSeparator))); | ||
| void DBImpl::CollectAllDBPaths() { |
There was a problem hiding this comment.
With this functionality factored into a function of a giant class, I would like to see enforcement of preconditions. Specifically, either check that it hasn't been called before, or allow repeated calls with no ill effect.
There was a problem hiding this comment.
I added a set empty before populating check.
db/db_impl/db_impl.h
Outdated
| @@ -1559,22 +1558,38 @@ class DBImpl : public DB { | |||
| // Assign db_id_ and write DB ID to manifest if necessary. | |||
| void SetDBId(std::string&& id, bool read_only, RecoveryContext* recovery_ctx); | |||
|
|
|||
| // Collect a deduplicated collection of paths used by this DB, including | |||
| // dbname_, DBOptions.db_paths, ColumnFamilyOptions.cf_paths. These paths | |||
| // cannot be dynamically changed, so they are collected once during DB | |||
There was a problem hiding this comment.
Is this true even if CreateColumnFamily is called?
There was a problem hiding this comment.
That's a great point. This is indeed not true when new column families are added during DB session, so this set needs to be refreshed there too. Thank you for catching this!
There was a problem hiding this comment.
I totally missed how column family operations would affect this set. The whole assumption that this set is fixed once DB initialized and is kept intact through the whole DB session is not true at all. So it's not useful to have a member variable storing them like this. Although I can refresh the member variable when column families are updated, the case for when column families are added are relatively straightforward but the case for when a column family becomes completely dereferenced is more nuanced.
So instead I just make the function a pure util function for collecting these paths on demand, it's just a code dedupe refactor.
jowlyzhang
force-pushed
the
discard_stall_on_open
branch
from
9184ac7
to
b3d3e3e
Compare
db/db_impl/db_impl_files.cc
Outdated
| @@ -1023,4 +997,47 @@ Status DBImpl::DeleteUnreferencedSstFiles(RecoveryContext* recovery_ctx) { | |||
| return s; | |||
| } | |||
|
|
|||
| void DBImpl::TrackExistingDataFiles( | |||
There was a problem hiding this comment.
Nit: Maybe move this to db_impl_open.cc since its only called by DBImpl::Open?
There was a problem hiding this comment.
That's a good call. Thank you for the suggestion!
jowlyzhang
force-pushed
the
discard_stall_on_open
branch
from
21cfb9f
to
347cad6
Compare
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Thank you for help review this change! |
|
@jowlyzhang merged this pull request in 2412530. |
This PR fix the issue that deletion of obsolete files during DB::Open are not rate limited.
The root cause is slow deletion is disabled if trash/db size ratio exceeds the configured
max_trash_db_ratiorocksdb/include/rocksdb/sst_file_manager.h
Line 126 in d610e14
In order for the deletion rate limiting logic to work properly, we should only start deleting files after
SstFileManagerhas finished tracking the whole DB, so the main fix is to move these two places that attempts to delete file after the tracking are done: 1) theDeleteScheduler::CleanupDirectorycall inSanitizeOptions, 2) theDB::DeleteObsoleteFilescall.There are some other aesthetic changes like refactoring collecting all the DB paths into a function, rename
DBImp::DeleteUnreferencedSstFilestoDBImpl:: MaybeUpdateNextFileNumberas it doesn't actually delete the files.Test plan:
Added unit test and verified with manual testing