Enable SecondaryCache::CreateFromString to create sec cache based on the uri for CompressedSecondaryCache #10132
Conversation
gitbw95
changed the title
gitbw95
changed the title
|
@gitbw95 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
| if (value.find("compressed_secondary_cache://") == 0) { | ||
| std::string args = value; | ||
| args.erase(0, std::strlen("compressed_secondary_cache://")); | ||
| Status status; | ||
| std::shared_ptr<SecondaryCache> sec_cache; | ||
|
|
There was a problem hiding this comment.
Is there a reason to do this here rather than through the ObjectRegistry? I believe almost everything is done that way.
| comp_sec_cache_options_type_info = { | ||
| {"capacity", | ||
| {offsetof(struct CompressedSecondaryCacheOptions, capacity), | ||
| OptionType::kSizeT, OptionVerificationType::kNormal, | ||
| OptionTypeFlags::kMutable}}, | ||
| {"num_shard_bits", | ||
| {offsetof(struct CompressedSecondaryCacheOptions, num_shard_bits), | ||
| OptionType::kInt, OptionVerificationType::kNormal, | ||
| OptionTypeFlags::kMutable}}, |
There was a problem hiding this comment.
Why not have a Cache as an option that can be configured internally rather than here? That way you could use alternative caches and more options easier.
There was a problem hiding this comment.
This CompressedSecondaryCache is a concrete implementation of SecondaryCache on top of LRUCache with its own CompressedSecondaryCacheOptions.
There was a problem hiding this comment.
@gitbw95 The CompressedSecondaryCache is a concrete implementation of a SecondaryCache that HAS a Cache inside of it. Currently, the constructor can only create an LRUCache here. However, internally the class supports any Cache implementation, such as ClockCache. The options you define here are a restricted subset of options for an LRUCache.
A cleaner way to implement this complete solution is:
- There is a class that has a Cache in it that implements the SecondaryCache interface
- There is a SecondaryCacheWrapper class that implements and has a SecondaryCache
- There is a CompressedSecondaryClass that extends SecondaryCacheWrapper and provides compression
These three classes solve everything you do here but are more extensible and provide more functionality. This separation would allow any Cache to be treated as a SecondaryCache and any SecondaryCache to be compressed.
Perhaps that separation is something that should be undertaken separately from this current effort?
There was a problem hiding this comment.
@mrambacher Thanks for the recommendation! The "cleaner way" can be one of the options to optimize current CompressedSecondaryCache in future, but it doesn't have the highest priority now. With the current CompressedSecondaryCache, we would like to firstly add stress tests, analyze and minimize memory fragmentations, analyze and prevent the re-compression of cold blocks, etc.
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
|
@gitbw95 has updated the pull request. You must reimport the pull request before landing. |
|
@gitbw95 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary:
Update SecondaryCache::CreateFromString and enable it to create sec cache based on the uri for CompressedSecondaryCache.
Test Plan:
Add unit tests.