v5.17 unconditionally spawns a thread on DB open which does nothing
- Dominant language
- C++
- Stars
- 32.1k
- Forks
- 6.9k
- Avg merge
- 32m
- Merged PRs (30d)
- 1
Description
Circa release 5.17 the following code was added to `rocksdb::SanitizeOptions()`
```
// Create a default SstFileManager for purposes of tracking compaction size
// and facilitating recovery from out of space errors.
if (result.sst_file_manager.get() == nullptr) {
std::shared_ptr sst_file_manager(
NewSstFileManager(result.env, result.info_log));
result.sst_file_manager = sst_file_manager;
}
```
forcing the creation of a `rocksdb::SstFileManager` which contains a `rocksdb::DeleteScheduler` member which then contains a call to `new rocksdb::port::Thread` which is already improperly typedef'ed to `std::thread` rather than using `Env` threading callbacks (see #4654). I don't see any condition which makes it possible to branch away from spawning this thread.
Casually spawning an `std::thread` is an expensive operation. It complicates the address space and my project's assumptions. It begs for trouble when working with things like musl, etc. It doesn't really fit RocksDB's minimalist style. If I wanted complications I'd just go link with `libmongo`. Maybe it's worth it though, depending on what we get; the \brief on `DeleteScheduler` tells us:
```
// DeleteScheduler allows the DB to enforce a rate limit on file deletion,
// Instead of deleteing files immediately, files are marked as trash
// and deleted in a background thread that apply sleep penlty between deletes
// if they are happening in a rate faster than rate_bytes_per_sec,
```
I think I'll pass on that. Since `rocksdb::SstFileManager` is a virtual interface like many parts of library there's a glimmer of hope for me to create a new `SstFileManager` implementation which avoids all of this. But if you [look](https://github.com/facebook/rocksdb/blob/b703a56e5cd722aaf169baa3e28127426776b6a9/db/db_impl/db_impl_open.cc#L1295) right in the middle of the important `DBImpl::Open()` function for the entire database you'll find:
```
static_cast(impl->immutable_db_options_.sst_file_manager.get());
```
That's a _static downcast of a virtual interface_ to your internal class. This makes the interface useless; literally not even useful as a wrapper for mere observation, much less an alternative implementation.
Everything going on here is trash.
Contributor guide
Assessment
This issue has not been assessed yet.