Fix type assertion in AdaptiveDirectoryCacheMaintainer #2525
Conversation
| @@ -14,14 +13,19 @@ internal class AdaptiveDirectoryCacheMaintainer<TValue> : AsynchAgent | |||
|
|
|||
| private readonly AdaptiveGrainDirectoryCache<TValue> cache; | |||
| private readonly LocalGrainDirectory router; | |||
| private readonly Func<List<ActivationAddress>, TValue> updateFunc; | |||
There was a problem hiding this comment.
Update function is not a great name, but can't think of a better one.
This pattern, over all, is awful, but not sure how to fix it, atm. :/
No change requested.. just making noises as bits of my soul die.
There was a problem hiding this comment.
Fair call. Originally I labelled it cacheUpdateFunc, but maybe cacheRefreshCallback or cacheMaintenanceCallback - I don't know. I'm not very familiar with this end of the codebase yet.
There was a problem hiding this comment.
By the way, I don't think the pattern here is that bad.
There was a problem hiding this comment.
As for the name, it seems to be a translation function. It translates the list of activation addresses to whatever the cache is storing. Since there are no assumptions may about the cached data there is no general way to translate, hence the need for a function. While this is not horrible, it definitely smells. Maybe using some sort of functor, like dictionaries use comparers, provided as a generic might be a cleaner approach. AdaptiveDirectoryCacheMaintainer<TValue,TTranslator>? In any case, this pattern at least deserves a look at for refactoring, but I understand that such is beyond the scope of this bugfix.
|
LGTM |
Fixes an error in
AdaptiveDirectoryCacheMaintainerwhich was performing a type check which would never succeed.TValuein the below code is alwaysIReadOnlyList<Tuple<SiloAddress, ActivationId>>, but the type check assumed it wasList<Tuple<SiloAddress, ActivationId>>.See the
//TODO:below for the source of the error.This removes the check and pushes the responsibility onto the caller.