facebook / facebook/rocksdb

Avoid exception if fail to insert block to block-cache

Open
#8,670 10 comments 1 reaction 1 assignee Claimed by @pdillinger View on GitHub
design discussion
Dominant language
C++
Stars
32.1k
Forks
6.9k
Avg merge
32m
Merged PRs (30d)
1

Description

### Expected behavior
If fail to insert block to block-cache, just abot the insert but return the result as normal.

### Actual behavior
If we use default `ReadOptions` with `strict_capacity_limit` block cache, RocksDB would throw exception if `Insert failed due to LRU cache being full`.

### Steps to reproduce the behavior
This is because current `RetrieveBlock` would [check the status of `MaybeReadBlockAndLoadToCache`](https://github.com/facebook/rocksdb/blob/add68bd28a512da751e2bdc612685fdeb7e6dde4/table/block_based/block_based_table_reader.cc#L1909-L1916):

~~~c
s = MaybeReadBlockAndLoadToCache(
prefetch_buffer, ro, handle, uncompression_dict, wait_for_cache,
block_entry, block_type, get_context, lookup_context,
/*contents=*/nullptr);

if (!s.ok()) {
return s;
}
~~~

And if fail to insert, the block cache would report [incomplete status](https://github.com/facebook/rocksdb/blob/add68bd28a512da751e2bdc612685fdeb7e6dde4/cache/lru_cache.cc#L326-L339):

~~~c
if ((usage_ + total_charge) > capacity_ &&
(strict_capacity_limit_ || handle == nullptr)) {
e->SetInCache(false);
if (handle == nullptr) {
// Don't insert the entry but still return ok, as if the entry inserted
// into cache and get evicted immediately.
last_reference_list.push_back(e);
} else {
if (free_handle_on_fail) {
delete[] reinterpret_cast(e);
*handle = nullptr;
}
s = Status::Incomplete("Insert failed due to LRU cache being full.");
}
~~~

As more and more applications move on cloud, and fine-granularity memory control is required in many use cases. I think enabling the strict capacity limit of block cache in production environment should be something valueable.
How about this solution:
1. Introduce a new status as `FailInsertCache`.
1. Introduce a new filed named `fill_cache_if_possible` in ReadOptions, which means if cannot fill block to cache, we would not treat `FailInsertCache` status as a problem, and continue the read process as normal.

What do you think of this problem and the propsed solution?

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.