-
Notifications
You must be signed in to change notification settings - Fork 1
Selki/ff3396 expiring bandit cache #128
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
Changes from all commits
d0093fb
ffc0c59
90429c1
e4d1d26
001ae6e
c386303
e5c85cc
4b02b3e
4bdfa7f
d4a14bd
1cf7391
64c6729
1d1f7ee
7336c3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { AbstractAssignmentCache } from './abstract-assignment-cache'; | ||
import { LRUCache } from './lru-cache'; | ||
|
||
/** | ||
* A cache that uses the LRU algorithm to evict the least recently used items. | ||
* | ||
* It is used to limit the size of the cache. | ||
* | ||
* The primary use case is for server-side SDKs, where the cache is shared across | ||
* multiple users. In this case, the cache size should be set to the maximum number | ||
* of users that can be active at the same time. | ||
* @param {number} maxSize - Maximum cache size | ||
*/ | ||
export class LRUInMemoryAssignmentCache extends AbstractAssignmentCache<LRUCache> { | ||
maya-the-mango marked this conversation as resolved.
Show resolved
Hide resolved
|
||
constructor(maxSize: number) { | ||
super(new LRUCache(maxSize)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { AbstractAssignmentCache } from './abstract-assignment-cache'; | ||
|
||
/** | ||
* A cache that never expires. | ||
* | ||
* The primary use case is for client-side SDKs, where the cache is only used | ||
* for a single user. | ||
*/ | ||
export class NonExpiringInMemoryAssignmentCache extends AbstractAssignmentCache< | ||
Map<string, string> | ||
> { | ||
constructor(store = new Map<string, string>()) { | ||
super(store); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,175 @@ | ||
import { TLRUCache } from './tlru-cache'; | ||
|
||
describe('TLRU Cache', () => { | ||
maya-the-mango marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let cache: TLRUCache; | ||
const expectedCacheTimeoutMs = 10; | ||
|
||
beforeEach(async () => { | ||
cache = new TLRUCache(2, expectedCacheTimeoutMs); | ||
}); | ||
|
||
afterEach(async () => { | ||
jest.restoreAllMocks(); | ||
jest.clearAllTimers(); | ||
}); | ||
|
||
it('should evict cache after expiration', () => { | ||
jest.useFakeTimers(); | ||
|
||
cache.set('a', 'apple'); | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs); | ||
|
||
expect(cache.get('a')).toBeUndefined(); | ||
}); | ||
|
||
it('should not evict cache before expiration', () => { | ||
jest.useFakeTimers(); | ||
|
||
cache.set('a', 'apple'); | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs - 1); | ||
expect(cache.get('a')).toBe('apple'); | ||
}); | ||
|
||
it('should evict all expired entries on .entries() call', () => { | ||
jest.useFakeTimers(); | ||
|
||
cache.set('a', 'avocado'); | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs); | ||
cache.set('b', 'banana'); | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs); | ||
|
||
const cacheEntries = []; | ||
|
||
for (const entry of cache.entries()) { | ||
cacheEntries.push(entry); | ||
} | ||
|
||
expect(cacheEntries.length).toBe(0); | ||
}); | ||
|
||
it('should evict all expired entries on .keys() call', () => { | ||
jest.useFakeTimers(); | ||
|
||
cache = new TLRUCache(3, expectedCacheTimeoutMs); | ||
cache.set('a', 'avocado'); | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs); | ||
cache.set('b', 'banana'); | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs); | ||
cache.set('c', 'cherry'); | ||
|
||
const cacheKeys = []; | ||
|
||
for (const key of cache.keys()) { | ||
cacheKeys.push(key); | ||
} | ||
|
||
expect(cacheKeys.length).toBe(1); | ||
expect(cache.get('c')).toBe('cherry'); | ||
Comment on lines
+66
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
}); | ||
|
||
it('should evict all expired entries on .values() call', () => { | ||
jest.useFakeTimers(); | ||
cache = new TLRUCache(3, expectedCacheTimeoutMs); | ||
|
||
cache.set('a', 'avocado'); | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs); | ||
cache.set('b', 'banana'); | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs); | ||
cache.set('c', 'cherry'); | ||
|
||
const cacheValues = []; | ||
|
||
for (const value of cache.values()) { | ||
cacheValues.push(value); | ||
} | ||
|
||
expect(cacheValues.length).toBe(1); | ||
expect(cache.get('c')).toBe('cherry'); | ||
Comment on lines
+86
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
}); | ||
|
||
it('should overwrite existing cache entry', () => { | ||
jest.useFakeTimers(); | ||
|
||
cache.set('a', 'apple'); | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs - 1); | ||
cache.set('a', 'avocado'); | ||
|
||
// spin the clock by 5sec. After that time cache entry should be still valid. | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs / 2); | ||
|
||
// setting assertion in a weird way because calling cache.get() | ||
// will reset eviction timer which will mess up next assertion | ||
let avocadoInCache = false; | ||
cache.forEach((value, key) => { | ||
if (key === 'a' && value === 'avocado') { | ||
avocadoInCache = true; | ||
} | ||
}); | ||
expect(avocadoInCache).toBe(true); | ||
|
||
// after another spin of 5 sec, cache entry should evict itself | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs / 2); | ||
expect(cache.get('a')).toBeUndefined(); | ||
}); | ||
|
||
it('should check if a key exists', () => { | ||
cache.set('a', 'apple'); | ||
expect(cache.has('a')).toBeTruthy(); | ||
expect(cache.has('b')).toBeFalsy(); | ||
}); | ||
|
||
it('should handle the cache capacity of zero', () => { | ||
const zeroCache = new TLRUCache(0, expectedCacheTimeoutMs); | ||
zeroCache.set('a', 'apple'); | ||
expect(zeroCache.get('a')).toBeFalsy(); | ||
}); | ||
|
||
it('should handle the cache capacity of one', () => { | ||
maya-the-mango marked this conversation as resolved.
Show resolved
Hide resolved
|
||
jest.useFakeTimers(); | ||
const oneCache = new TLRUCache(1, expectedCacheTimeoutMs); | ||
oneCache.set('a', 'apple'); | ||
jest.advanceTimersByTime(expectedCacheTimeoutMs); | ||
expect(oneCache.get('a')).toBeUndefined(); | ||
|
||
oneCache.set('a', 'avocado'); | ||
expect(oneCache.get('a')).toBe('avocado'); | ||
oneCache.set('b', 'banana'); | ||
expect(oneCache.get('a')).toBeFalsy(); | ||
expect(oneCache.get('b')).toBe('banana'); | ||
}); | ||
|
||
it('should evict oldest entry when capacity limit is reached', () => { | ||
cache.set('a', 'apple'); | ||
cache.set('b', 'banana'); | ||
cache.set('c', 'cherry'); | ||
|
||
expect(cache.get('a')).toBeUndefined(); | ||
expect(cache.has('b')).toBeTruthy(); | ||
expect(cache.has('c')).toBeTruthy(); | ||
}); | ||
|
||
/** | ||
This test case might be an overkill but in case Map() changes, | ||
or we want to ditch it completely this will remind us that insertion | ||
order is crucial for this cache to work properly | ||
**/ | ||
it('should preserve insertion order when inserting on capacity limit', () => { | ||
cache.set('a', 'apple'); | ||
cache.set('b', 'banana'); | ||
cache.set('c', 'cherry'); | ||
|
||
let keys = Array.from(cache.keys()); | ||
expect(keys[0]).toBe('b'); | ||
expect(keys[1]).toBe('c'); | ||
|
||
cache = new TLRUCache(2, expectedCacheTimeoutMs); | ||
cache.set('a', 'apple'); | ||
cache.set('b', 'banana'); | ||
cache.get('a'); | ||
cache.set('c', 'cherry'); | ||
|
||
keys = Array.from(cache.keys()); | ||
expect(keys[0]).toBe('a'); | ||
expect(keys[1]).toBe('c'); | ||
Comment on lines
+171
to
+173
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💪 |
||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need another test that does:
Then makes sure its |
||
}); |
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.
probably handled by a test like your "should evict oldest entry when capacity limit is reached" but no harm no foul