feat(#169): implement cache expiry for remote places #237
Conversation
| @@ -31,8 +24,38 @@ export type RemotePlace = { | |||
| }; | |||
|
|
|||
| export default class RemotePlaceCache { | |||
| private static cache: RemotePlaceDatastore = {}; | |||
| private static cache: NodeCache; | |||
| private static readonly CACHE_TTL = 3600; | |||
There was a problem hiding this comment.
is this one hour? i suspect we want it to be like 24 hrs? can you share your thinking? for nairobi i think this takes over a minute and 1 hour expiration would guarantee basically users do it once per session. is that what we want?
| } | ||
|
|
||
| private static getCache(): NodeCache { | ||
| if (!this.cache) { |
There was a problem hiding this comment.
it seems easy to use RemotePlaceCache.cache instead of getCache? Is to make that harmless? Like calling initialize once RemotePlaceCache.initialize() once within the module scope and then you can use this.cache safely everywhere else
| private static readonly CACHE_TTL = 3600; | ||
| private static readonly CACHE_CHECK_PERIOD = 120; | ||
|
|
||
| public static async getPlacesWithType(chtApi: ChtApi, contactType: ContactType, hierarchyLevel: HierarchyConstraint) |
There was a problem hiding this comment.
what the difference between public getPlacesWithType and public getRemotePlaces in this class? are two interfaces like this required?
| // Clear all keys matching domain prefix | ||
| const keys = this.getCache().keys(); | ||
| const domainPrefix = `${domain}:`; | ||
| keys.forEach(key => { |
There was a problem hiding this comment.
| keys.forEach(key => { | |
| keys.filter(key => key.startsWith(domainPrefix)) | |
| .forEach(this.cache.del) |
Consider ?
| // if there is no cache existing, discard the value | ||
| // it will be fetched if needed when the cache is built | ||
| if (places) { | ||
| const cacheKey = this.getCacheKey(domain, placeType); |
There was a problem hiding this comment.
lets say there is a place x of type type on the instance.
scenario 1
- the cache is empty
- run get('type')
you'll get x
but scenario 2
- the cache is empty
- add(y) of
type - get('type')
now you won't see x. i think this is a bug, no?
|
|
||
| const places = this.getCache().get<RemotePlace[]>(cacheKey) || []; | ||
|
|
||
| if (places.length === 0) { |
There was a problem hiding this comment.
why do you only add if places is empty? previously, we only added if not empty...
| @@ -118,4 +152,11 @@ export default class RemotePlaceCache { | |||
|
|
|||
| return []; | |||
| } | |||
|
|
|||
| // For testing purposes | |||
| public static hasData(domain: string, placeType: string): boolean { | |||
There was a problem hiding this comment.
you can just access private members in tests. shouldn't this be a function within the spec.ts file?
Replace PR#183
closes #169
Note: Failing unit test will be fix by PR#239