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

feat: Added a generic LRUCache interface and a default implementation #311

Merged
merged 21 commits into from
Aug 17, 2022

Conversation

mikechu-optimizely
Copy link
Contributor

Summary

Added an interface for a LRU Caching mechanism using generics. Include a default implementation.

Test plan

Included new tests in the test project

Issues

OASIS-8407

@mikechu-optimizely mikechu-optimizely requested a review from a team as a code owner August 8, 2022 16:48
@mikechu-optimizely mikechu-optimizely changed the title feat: feat: Added a generic LRUCache interface and a default implementation Aug 8, 2022
@mikechu-optimizely mikechu-optimizely requested review from jaeopt and msohailhussain and removed request for a team August 10, 2022 17:04
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

OptimizelySDK/Odp/ILruCache.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/LruCache.cs Outdated Show resolved Hide resolved
@mikechu-optimizely
Copy link
Contributor Author

@jaeopt Would you like for this Pull Request to also wait for @msohailhussain 's approval?

@jaeopt
Copy link
Contributor

jaeopt commented Aug 10, 2022

@jaeopt Would you like for this Pull Request to also wait for @msohailhussain 's approval?

Yes for all PRs. Unfortunately, I cannot cover csharp lang stuffs :)

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please address

OptimizelySDK.Tests/OdpTests/CollectionExtensions.cs Outdated Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/CollectionExtensions.cs Outdated Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/LruCacheTest.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/DateTimeExtension.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/ICache.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/LruCache.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/LruCache.cs Outdated Show resolved Hide resolved
{
var cache = new LruCache<List<string>>(0, LruCache<object>.DEFAULT_TIMEOUT_SECONDS);

cache.Save("user1", _segments1And2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this possible if we can return true or false. @jaeopt a question for you?

OptimizelySDK/Odp/LruCache.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/LruCache.cs Outdated Show resolved Hide resolved
@mikechu-optimizely
Copy link
Contributor Author

@msohailhussain Can you take a look at this Pull Request and the suggested LRU implementation when you have a chance?

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have some questions.

OptimizelySDK/Odp/LruCache.cs Show resolved Hide resolved
OptimizelySDK/Odp/LruCache.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/LruCache.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/LruCache.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/LruCache.cs Show resolved Hide resolved
_logger = logger ?? new DefaultLogger();

_cache =
new Dictionary<string, (LinkedListNode<string> node, ItemWrapper value)>(_maxSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am just curious can't we just take LinkedListNode as value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea is to pop off both the node from the Tuple and put it in the _list & back in the _cache along with the item see linw 150ish.

Let's talk through this too.

OptimizelySDK/Odp/LruCache.cs Outdated Show resolved Hide resolved
@mikechu-optimizely
Copy link
Contributor Author

@msohailhussain This is ready for another review after our conversation.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mikechu-optimizely mikechu-optimizely merged commit 08b97d2 into master Aug 17, 2022
@mikechu-optimizely mikechu-optimizely deleted the mike/lru-cache branch August 17, 2022 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants