refactor(sdk): StreamRegistry cache invalidation
#2885
Conversation
teogeb
changed the title
StreamRegistry#populateMetadataCache()StreamRegistry cache invalidation
| if (!allowCached) { | ||
| invalidateCache(this.metadataCache, streamId) | ||
| } | ||
| return this.metadataCache.get(streamId) |
There was a problem hiding this comment.
The way this used to work is that when setting useCache = false it would just do a direct call and not use cache. Now it seems to be clearing cache. Is there a problematic scenario where if the underlying call rejects (e.g. JSON RPC is down) we clear a cache but can't provide a replace value?
Comment applies to later usages in this PR as well.
There was a problem hiding this comment.
Yes, that was described in the "small functionality changes" section. Calling the function with allowCached = false indicates that the caller needs the value to be up-to-date. Before this change one component could have seen that stream metadata is foo, and another that it is bar. The current application wide consistency is most likely better.
I.e. it follows the principle that it is better to fail a task than to allow data corruption to happen.
| @@ -18,14 +24,11 @@ export class CachingMap<K extends MapKey, V, P extends any[]> { | |||
|
|
|||
| private readonly cachedFn: (...args: P) => Promise<V> | |||
| private readonly cache: LRU<K, { data: V, maxAge: number }> | |||
| private readonly opts: Options<P, K> | |||
There was a problem hiding this comment.
(nitpick) A Readonly<> here could provide more clarity to the reader that the options are not meant to change during the life of an instance of CachingMap.
| private readonly opts: Options<P, K> | |
| private readonly opts: Readonly<Options<P, K>> |
There was a problem hiding this comment.
We haven't used this kind of deep annotation in the sdk code base before.
Many of our objects are immutable, but adding many Readonly<> annotations to all of those would just decrease readability. Using readonly in the field type is not the same, but it communicates the intention quite well.
(Also the name and the type of the field should communicate clearly that we are using the Configuration Option Pattern which implies immutability by default.)
Refactor
StreamRegistrycaching:populateMetadataCache()method to populate the cacheinvalidateCache()helper function to simplify cache invalidationSmall functionality changes
setStreamMetadata()andcreateStream()calls (we can easily populate the cache as the metadata is a method parameter)allowCached == false, the value is stored to cache. Pre-existing value is invalidated (i.e. possible concurrent calls will also get fresh data).