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.

Sign up for GitHub

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

Fix type assertion in AdaptiveDirectoryCacheMaintainer #2525

Merged
merged 1 commit into from
Dec 16, 2016
Merged

Fix type assertion in AdaptiveDirectoryCacheMaintainer #2525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions src/OrleansRuntime/GrainDirectory/AdaptiveDirectoryCacheMaintainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using Orleans.Runtime.Scheduler;

Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I don't think the pattern here is that bad.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private long lastNumAccesses; // for stats
private long lastNumHits; // for stats

internal AdaptiveDirectoryCacheMaintainer(ILocalGrainDirectory router, IGrainDirectoryCache<TValue> cache)
internal AdaptiveDirectoryCacheMaintainer(
LocalGrainDirectory router,
AdaptiveGrainDirectoryCache<TValue> cache,
Func<List<ActivationAddress>, TValue> updateFunc)
{
this.router = (LocalGrainDirectory)router;
this.cache = (AdaptiveGrainDirectoryCache<TValue>) cache;
this.updateFunc = updateFunc;
this.router = router;
this.cache = cache;

lastNumAccesses = 0;
lastNumHits = 0;
Expand Down Expand Up @@ -88,7 +92,7 @@ protected override void Run()
}
else if (entry.NumAccesses == 0)
{
// 2. If the entry is expired and was not acccessed in the last time interval -- throw it away
// 2. If the entry is expired and was not accessed in the last time interval -- throw it away
cache.Remove(grain); // for debug
cnt3++;
}
Expand Down Expand Up @@ -149,17 +153,14 @@ private void ProcessCacheRefreshResponse(

int cnt1 = 0, cnt2 = 0, cnt3 = 0;

//TODO: this is unsafe in case TValue changes in the future as it assumes List<Tuple<SiloAddress, ActivationId>>
var cacheRef = cache as AdaptiveGrainDirectoryCache<List<Tuple<SiloAddress, ActivationId>>>;

// pass through returned results and update the cache if needed
foreach (Tuple<GrainId, int, List<ActivationAddress>> tuple in refreshResponse)
{
if (tuple.Item3 != null)
{
// the server returned an updated entry
var list = tuple.Item3.Select(a => Tuple.Create(a.Silo, a.Activation)).ToList();
cacheRef.AddOrUpdate(tuple.Item1, list, tuple.Item2);
var updated = updateFunc(tuple.Item3);
cache.AddOrUpdate(tuple.Item1, updated, tuple.Item2);
cnt1++;
}
else if (tuple.Item2 == -1)
Expand Down
11 changes: 8 additions & 3 deletions src/OrleansRuntime/GrainDirectory/GrainDirectoryCacheFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,15 @@ internal static IGrainDirectoryCache<TValue> CreateGrainDirectoryCache(GlobalCon
}
}

internal static AsynchAgent CreateGrainDirectoryCacheMaintainer(LocalGrainDirectory router, IGrainDirectoryCache<TValue> cache)
internal static AsynchAgent CreateGrainDirectoryCacheMaintainer(
LocalGrainDirectory router,
IGrainDirectoryCache<TValue> cache,
Func<List<ActivationAddress>, TValue> updateFunc)
{
return cache is AdaptiveGrainDirectoryCache<TValue> ?
new AdaptiveDirectoryCacheMaintainer<TValue>(router, cache) : null;
var adaptiveCache = cache as AdaptiveGrainDirectoryCache<TValue>;
return adaptiveCache != null
? new AdaptiveDirectoryCacheMaintainer<TValue>(router, adaptiveCache, updateFunc)
: null;
}
}

Expand Down
6 changes: 5 additions & 1 deletion src/OrleansRuntime/GrainDirectory/LocalGrainDirectory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,11 @@ public LocalGrainDirectory(ClusterConfiguration clusterConfig, SiloInitializatio
DirectoryCache = GrainDirectoryCacheFactory<IReadOnlyList<Tuple<SiloAddress, ActivationId>>>.CreateGrainDirectoryCache(globalConfig);
}
});
maintainer = GrainDirectoryCacheFactory<IReadOnlyList<Tuple<SiloAddress, ActivationId>>>.CreateGrainDirectoryCacheMaintainer(this, this.DirectoryCache);
maintainer =
GrainDirectoryCacheFactory<IReadOnlyList<Tuple<SiloAddress, ActivationId>>>.CreateGrainDirectoryCacheMaintainer(
this,
this.DirectoryCache,
activations => activations.Select(a => Tuple.Create(a.Silo, a.Activation)).ToList().AsReadOnly());
GsiActivationMaintainer = new GlobalSingleInstanceActivationMaintainer(this, this.Logger, globalConfig);

if (globalConfig.SeedNodes.Count > 0)
Expand Down