facebook / facebook/rocksdb

v5.17 unconditionally spawns a thread on DB open which does nothing

Open
#5,466 0 comments 0 reactions 0 assignees View on GitHub
up-for-grabs
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.