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

InMemoryCache #2

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

InMemoryCache #2

wants to merge 9 commits into from

Conversation

devanshu0987
Copy link

Design a Cache with LRU cache eviction policy.

  1. Cache should support get(), put methods.
  2. For simplicity, all keys and values are strings.
  3. Make it extendable to support multiple other eviction policies in the future.

Copy link
Owner

@kousiknath kousiknath left a 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 {
Copy link
Owner

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.

src/com/lld/inmemorycache/model/IEvictionPolicy.java Outdated Show resolved Hide resolved
@@ -0,0 +1,11 @@
package com.lld.inmemorycache.model;

public abstract class AbstractCache {
Copy link
Owner

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.
Copy link
Owner

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 {
Copy link
Owner

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>

src/com/lld/inmemorycache/model/impl/InMemoryStorage.java Outdated Show resolved Hide resolved
lock.lock();
try {
// access _storage. do not allow an exception here.
_storage.remove(key);
Copy link
Owner

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();
Copy link
Owner

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<>();
Copy link
Owner

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)
Copy link
Owner

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.

@kousiknath
Copy link
Owner

@devanshu0987 would it be possible for you to address the comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants