Fix bugs in Simple Cache #263
Conversation
raakella1
commented
Mar 30, 2025
•
raakella1
commented
- Fix the condition to evict keys in lru_evictor
- Remove the key from hashmap after removing from the evictor.
- Add a test case to trigger eviction using multiple threads.
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #263 +/- ##
==========================================
- Coverage 64.29% 60.79% -3.51%
==========================================
Files 72 74 +2
Lines 4406 4828 +422
Branches 555 654 +99
==========================================
+ Hits 2833 2935 +102
- Misses 1327 1634 +307
- Partials 246 259 +13
🚀 New features to boost your workflow:
|
raakella1
force-pushed
the
simple_cache
branch
3 times, most recently
from
b3ff5d7 to
d3ca31b
Compare
| @@ -58,13 +58,19 @@ bool LRUEvictor::LRUPartition::add_record(CacheRecord &record) { | |||
|
|
|||
| void LRUEvictor::LRUPartition::remove_record(CacheRecord &record) { | |||
| std::unique_lock guard{m_list_guard}; | |||
| if (!record.m_member_hook.is_linked()) { | |||
There was a problem hiding this comment.
can you put some comment here, what does this if check and why should we return here?
There was a problem hiding this comment.
However why cache would remove a record that is not present, shouldn't we crash to catch the bug?
There was a problem hiding this comment.
I was thinking Debug assert and error log? It would not affect functionality in production, just a size mismatch
There was a problem hiding this comment.
This check is this is here for a reason right? Or is it just a defend check? If we are seeing this assert in our test, it means there is probably some race we don't know yet?
There was a problem hiding this comment.
This check was not there before, I added it because I hit the crash due to a bug in my implementation that was hit during the unit test. I added this now so that we will know where it failed
There was a problem hiding this comment.
This is just a defend check, we are not hitting it now in the tests
|
@yamingk I added more comments to the code at several places you suggested |
