-
Notifications
You must be signed in to change notification settings - Fork 68
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
chore: Implement custom LRU cache with protected keys #3097
chore: Implement custom LRU cache with protected keys #3097
Conversation
Signed-off-by: Victor Yanev <[email protected]>
Signed-off-by: Victor Yanev <[email protected]>
🚨 Memory Leak Detected 🚨A potential memory leak has been detected in the test titled Details📊 Memory Leak Detection Report 📊 GC Type: MarkSweepCompact Heap Statistics (before vs after executing the test):
Heap Space Statistics (before vs after executing the test):
RecommendationsPlease investigate the memory allocations in this test, focusing on objects that are not being properly deallocated. |
Signed-off-by: Victor Yanev <[email protected]>
Signed-off-by: Victor Yanev <[email protected]>
Signed-off-by: Victor Yanev <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work,
some suggestions
Signed-off-by: Victor Yanev <[email protected]>
Signed-off-by: Victor Yanev <[email protected]>
* @private | ||
*/ | ||
private isPreconfiguredPlanKey(key: string, plan: SpendingPlanConfig): boolean { | ||
return key.includes(`${HbarSpendingPlanRepository.collectionKey}:${plan.id}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This should work since we're working with fixed length keys. In general I would try to get an exact match, in the case that a key contains the substring of another key.
const expectedKey = `${HbarSpendingPlanRepository.collectionKey}:${plan.id}`;
return key === expectedKey;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentionally done here, because we want to also match the amountSpent
and spendingHistory
keys which contain the ${HbarSpendingPlanRepository.collectionKey}:${plan.id}
prefix:
${HbarSpendingPlanRepository.collectionKey}:${plan.id}:amountSpent
${HbarSpendingPlanRepository.collectionKey}:${plan.id}:spendingHistory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to a more general approach where we initialize a set of protected keys in the constructor and then in delete
we check for the existence of the key in this set of protected keys:
delete(key: K): boolean {
if (this.protectedKeys.has(key)) {
this.logger.trace(`Deletion of key ${key} is ignored as it is protected.`);
return false;
}
return super.delete(key);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. I have a couple of questions.
Signed-off-by: Victor Yanev <[email protected]>
Quality Gate passedIssues Measures |
Looks like this approach is still not working for scenarios where the beforeEach(() => {
cache = new CustomLRUCache<string, any>(logger, { ttl: 100, max: 3 });
});
...
// ⚠️ ⚠️ ⚠️ THIS TEST IS FAILING ⚠️ ⚠️ ⚠️
it('should not delete a protected key when the cache is full', () => {
const protectedKey = `hbarSpendingPlan:${spendingPlansConfig[0].id}`;
const keys = ['key1', 'key2', protectedKey];
keys.forEach((key) => cache.set(key, 'value'));
cache.get('key1');
cache.get('key2');
cache.set('key3', 'value');
expect(cache.has(protectedKey)).to.be.true;
}); And therefore, unfortunately, this PR is pointless... |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3097 +/- ##
==========================================
+ Coverage 83.16% 85.11% +1.94%
==========================================
Files 63 45 -18
Lines 4236 3372 -864
Branches 830 669 -161
==========================================
- Hits 3523 2870 -653
+ Misses 470 294 -176
+ Partials 243 208 -35
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Replaced by #3140 |
Description:
This PR implements a custom
LRUCache
implementation with the ability to protect certain keys from being deleted. The cache is designed to handle specific key prefixes that are associated with pre-configured spending plans, ensuring that these keys are not removed from the cache.Changes
hbarSpendingPlan:{{id}}
,ethAddressHbarSpendingPlan:{{ethAddress}}
,ipAddressHbarSpendingPlan:{{ipAddress}}
).Unit Tests:
customLRUCache.spec.ts
to verify the behavior of theCustomLRUCache
.Related issue(s):
Fixes #3096
Notes for reviewer:
Checklist