-
Notifications
You must be signed in to change notification settings - Fork 31
Add a .Net Core distributed cache #29
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
Add a .Net Core distributed cache #29
Conversation
fd864b3
to
782d70c
Compare
{ | ||
// Like IMemoryCache, it does not support Clear. Unlike it, it does neither provides a dependency | ||
// mechanism which would allow to implement it. | ||
Log.Warn($"Clear is not supported by {nameof(IDistributedCache)}, ignoring the call."); |
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.
Yuck!
/// <remarks> | ||
/// Changes to this property affect only caches built after the change. | ||
/// </remarks> | ||
public static ICoreDistributedCacheFactory CacheFactory { get; set; } |
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.
See this comment on this PR matching issue.
782d70c
to
5b49b33
Compare
5bbfdd9
to
9c70599
Compare
I may add two "companion" packages, one for a Redis factory and the other for SQL Server distributed cache. |
01c3f97
to
a210c88
Compare
dda3452
to
6c1e1f1
Compare
Last commit was causing tests to fail, but this was not detected by the CI script. Now the test failure is fixed (and the factories names slightly simplified by the way). |
0b9693e
to
c0350b9
Compare
84acd34
to
baf0c87
Compare
WIP again: going to add a Memcached factory, and adjust the factory interface for being a bit more adaptable to new needs without breaking changes. |
c325393
to
8b5be8b
Compare
foreach (var forbidden in ForbiddenChar) | ||
{ | ||
key = key.Replace(forbidden, '-'); | ||
} |
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.
I have checked two other implementations: Regex
and StringBuilder
. They were no match in execution time, so keeping this basic Replace
one.
For reference, here is how I have checked:
[Test]
public void PerfTest()
{
var keys = new[]
{
"abcdefghijklmnopkrstuvwxyz",
"abcde\fghijklm\nopk\r \tu\vwxyz",
"abcdefghijklmnopkrstuvwxyzabcdefghijklmnopkrstuvwxyzabcdefghijklmnopkrstuvwxyzabcdefghijklmnopkrstuvwxyz" +
"abcdefghijklmnopkrstuvwxyzabcdefghijklmnopkrstuvwxyzabcdefghijklmnopkrstuvwxyzabcdefghijklmnopkrstuvwxyz",
"abcde\fghijklm\nopk\r \tu\vwxyzabcde\fghijklm\nopk\r \tu\vwxyzabcde\fghijklm\nopk\r \tu\vwxyzabcde\fghijklm\nopk\r \tu\vwxyz" +
"abcde\fghijklm\nopk\r \tu\vwxyzabcde\fghijklm\nopk\r \tu\vwxyzabcde\fghijklm\nopk\r \tu\vwxyzabcde\fghijklm\nopk\r \tu\vwxyz"
};
var sw = new Stopwatch();
Regex purgeForbiddenChar = null;
HashSet<char> forbiddenCharSet = null;
char[] forbiddenChar = null;
var algos = new[]
{
new Tuple<string, System.Action, Func<string, string>>("Regex",
() => purgeForbiddenChar = new Regex("[ \n\r\t\v\f]", RegexOptions.Compiled),
s => purgeForbiddenChar.Replace(s, "-")),
new Tuple<string, System.Action, Func<string, string>>("Builder",
() => forbiddenCharSet = new HashSet<char> { ' ', '\n', '\r', '\t', '\v', '\f' },
s =>
{
var sanitizedKey = new StringBuilder();
foreach (var c in s)
{
sanitizedKey.Append(forbiddenCharSet.Contains(c) ? '-' : c);
}
return sanitizedKey.ToString();
}),
new Tuple<string, System.Action, Func<string, string>>("Replace",
() => forbiddenChar = new[] { '\n', ' ', '\r', '\t', '\v', '\f' },
s =>
{
foreach (var forbidden in forbiddenChar)
{
s = s.Replace(forbidden, '-');
}
return s;
})
};
foreach (var algo in algos)
{
sw.Restart();
algo.Item2();
foreach (var k in keys)
{
Console.WriteLine(algo.Item3(k));
}
Console.WriteLine($"{sw.Elapsed} for {algo.Item1} warmup");
foreach (var k in keys)
{
sw.Restart();
for (var i = 0; i < 1000; i++)
{
algo.Item3(k + i);
}
Console.WriteLine($"{sw.Elapsed} for {algo.Item1} key {k}");
}
}
}
|
||
var loggerFactory = new LoggerFactory(); | ||
|
||
_cache = new MemcachedClient(loggerFactory, new MemcachedClientConfiguration(loggerFactory, options)); |
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.
It is a bit messy to integrate a library meant to be used with the .Net Core default dependency injection system with NHibernate factories/configuration logic.
EnyimMemcachedCore was the worst case, due to its dependency on a logger factory (I have provided it through a wrapping of the NHibernate one) and its structured configuration (thus the JSON trick I have choosen for it).
Maybe should each factory also expose directly a constructor with the cache dependencies (at least IMemcachedClientConfiguration
and maybe ILoggerFactory
for this case)for users who would set the factory programmatically.
<!-- Targeting net461 explicitly in order to avoid https://github.com/dotnet/standard/issues/506 for net461 consumers--> | ||
<TargetFrameworks>net461;netstandard2.0</TargetFrameworks> | ||
<NoWarn>$(NoWarn);3001;3002</NoWarn> | ||
<SignAssembly>False</SignAssembly> |
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.
EnyimMemcachedCore does not have strong name. This creates no issues for netcoreapp2.0 tests and compilation whatever the target, but if we sign NHibernate.Caches.CoreDistributedCache.Memcached, then net461 tests fail loading the dependency, complaining System.IO.FileLoadException : Could not load file or assembly 'EnyimMemcachedCore, Version=2.1.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies. A strongly-named assembly is required.
It seems it is recommended to no more strongly name assemblies in most cases, so not signing this package should be acceptable.
{ | ||
// According to https://docs.microsoft.com/en-us/aspnet/core/performance/caching/distributed#the-idistributedcache-interface | ||
// (see its paragraph end note) there is no need for a singleton lifetime. | ||
return new RedisCache(_options); |
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.
I wonder if we can and should actually trust this. NHibernate creates many caches, at least one by region which could mean one per mapped collection (and indeed much more, see nhibernate/nhibernate-core#1480).
Each RedisCache
hold its own Redis "database" instance. If that mean some kind of permanently opened connection on Redis, that would be bad to create many instances of RedisCache
.
{ | ||
// According to https://docs.microsoft.com/en-us/aspnet/core/performance/caching/distributed#the-idistributedcache-interface | ||
// (see its paragraph end note) there is no need for a singleton lifetime. | ||
return new SqlServerCache(_options); |
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.
Same question than for RedisCache
, but with a different concern: each SqlServerCache
may spawn its own background task for deleting expired items, at each operation on itself, if enough time has elapsed since the last time the instance have done it. So the more instances we create, the more such background task may be run concurrently. Moreover this causes the expired items deletion to occur overall much more frequently than asked through ExpiredItemsDeletionInterval
.
After that, for information there are two more things I intend to do (within their own PR): generate the xml documentation (at least for NuGet packages) and generate release artifacts on AppVeyor. Then I will proceed with releasing caches. Update: now done and this PR is rebased. |
40fd4a0
to
a310428
Compare
This implementation is not standalone: an IDistributedCache implementation has to be provided through an IDistributedCacheFactory, to be implemented by the user. Fix nhibernate#28
And provide by default the MemoryDistributedCacheFactory. To be squashed.
* Move the MemoryDistributedCacheFactory to its own package for avoiding an undue dependency. * Add a RedisCache factory. * Add a SqlServerCache factory. To be squashed.
Fixes some erroneous comments by the way. Handle cls non compliant cases one by one instead of globally disabling the warning. To be squashed, like all other commits of this PR.
a310428
to
b597957
Compare
I intend to merge and publish 5.3 next monday, unless someone wants to take the time reviewing this pending PR. |
This implementation is not standalone: an IDistributedCache
implementation has to be provided through an IDistributedCacheFactory.
Four such factories are provided as additional packages, covering the three caches from https://github.com/aspnet/Caching and Memcached through EnyimMemcachedCore. (Other implementations exist.)
Fix #28