-
Notifications
You must be signed in to change notification settings - Fork 73
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
InMemoryCache #2
base: main
Are you sure you want to change the base?
Conversation
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.
Organize the files properly so that not every file resides inside model
package.
@@ -0,0 +1,9 @@ | |||
package com.lld.inmemorycache.model; | |||
|
|||
public interface IEvictionPolicy { |
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.
Although the naming convention for interfaces starting with I
is valid and mostly this convention is used in C++ or C#, in this repo, we have used the convention as following:
Interface - MyService
Class - MyServiceImpl
under a impl
package in the same directory as the interfaces.
Please follow the convention to make the repo consistent.
@@ -0,0 +1,11 @@ | |||
package com.lld.inmemorycache.model; | |||
|
|||
public abstract class AbstractCache { |
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.
Instead of having both the key and value as String
type, it would be better if the design supports key of type <K extends Comparable<K>>
and value of type V
.
|
||
1. Cache should support get(), put methods. | ||
2. For simplicity, all keys and values are string. | ||
3. Make it extendable to support multiple other eviction policies in the future. |
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.
More could be added like support generic key value pairs, concurrency.
@@ -0,0 +1,8 @@ | |||
package com.lld.inmemorycache.model; | |||
|
|||
public interface IStorage { |
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.
public interface Storage<K, V>
lock.lock(); | ||
try { | ||
// access _storage. do not allow an exception here. | ||
_storage.remove(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.
What exception is thrown here?
|
||
@Override | ||
public void keyAccessed(String key) { | ||
lock.lock(); |
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.
As mentioned, taking individual locks at the storage or eviction layer won't work.
|
||
public LRUEvictionPolicy() { | ||
keys = new DoublyLinkedList(); | ||
mapper = new LinkedHashMap<>(); |
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.
Looks like managing a hash map here just to find out if the eviction policy has a key or not is an overkill. Already storage layer is maintaining another hash map. If we maintain a map here, it just doubles the space with not much gain. Probably we can simply remove / get the key from the double linked list when a key is accessed or removed from the cache and return either the node or null
to differentiate whether the operation was successful.
|
||
public class MainApplication { | ||
|
||
public static void main(String[] args) |
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.
Suggest writing multiple tests simulating multiple scenarios.
@devanshu0987 would it be possible for you to address the comments? |
Design a Cache with LRU cache eviction policy.