Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: investigate interaction between SST sideloading and raft entry cache #31688

Closed
tbg opened this issue Oct 22, 2018 · 2 comments
Closed
Labels
A-kv-replication Relating to Raft, consensus, and coordination. C-performance Perf of queries or internals. Solution not expected to change functional behavior.

Comments

@tbg
Copy link
Member

tbg commented Oct 22, 2018

Currently we use the Raft entry cache to hold on to the SSTable payloads when they are inlined as this saves roundtrips to disk. However, since the entry cache is shared across the store, workloads running concurrently with a RESTORE operation could see their performance severely degraded.

@nvanbenschoten nvanbenschoten added C-performance Perf of queries or internals. Solution not expected to change functional behavior. A-kv-replication Relating to Raft, consensus, and coordination. labels Oct 22, 2018
@nvanbenschoten
Copy link
Member

I suspect that this will result in comically bad behavior. The Raft entry with the sideloaded SST will come along and flush the entire cache, evicting everything so that they all need to be read from disk during application. Then a single entry from a different Range will come along and flush the sideloaded SST entry before it applies, rendering the entire process of caching the inlined entry completely useless.

I think the best fix would be to cache only thin SST entries. This will result in an extra disk read to pull the SST into memory when it is applied, but that's a fixed cost and it will avoid the current behavior of needlessly hurting other concurrent work.

@nvanbenschoten
Copy link
Member

This was fixed by the new raftentry.Cache, which refuses to cache Raft entries above 16 MB. These large entries no longer cause all other entries to be evicted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-replication Relating to Raft, consensus, and coordination. C-performance Perf of queries or internals. Solution not expected to change functional behavior.
Projects
None yet
Development

No branches or pull requests

2 participants