Replace ICache interface by a CacheBase class #1777

fredericDelaporte · 2018-06-27T13:44:26Z

In order to ease adding cache features, and provide default implementations for them, use an abstract base class instead of an interface.

This is similar to DbConnection, DbCommand and the like pattern.

It allows to reduce the needs for "transitional interfaces" and simplifies implementing new features as it allows to abstract away whether the implementation is obsolete or not, actually supports the new features or not.

fredericDelaporte · 2018-06-27T13:49:28Z

src/NHibernate/Async/Cache/FakeCache.cs

 	{

-		public Task<object> GetAsync(object key, CancellationToken cancellationToken)
+		public override Task<object> GetAsync(object key, CancellationToken cancellationToken)


These overrides are not really needed since the base do the same, but I guess it would be a bit hard for the async generator to detect that case.

fredericDelaporte · 2018-06-27T13:51:47Z

src/NHibernate/Cache/CacheBase.cs

+	/// </para>
+	/// <para>
+	/// This base class provides minimal async method implementations delegating their work to their
+	/// synchronous counterparts. Override them for supplying actual async operations.


Same pattern as DbConnection: a default async implementation calling the synchronous methods is provided for relieving implementors of doing that themselves, if their cache has no async operations.

src/NHibernate/Cache/CacheBase.cs

fredericDelaporte · 2018-06-27T14:08:01Z

src/NHibernate/Cache/CacheFactory.cs

 			try
 			{
-				impl = settings.CacheProvider.BuildCache(name, properties);
+				ccs.Cache = settings.CacheProvider.BuildCache(name, properties);


This property setter has no logic, doing the assignment outside of the try was just complicating the code for no reasons.

fredericDelaporte · 2018-06-27T14:09:07Z

src/NHibernate/Cache/CacheFactory.cs

 			try
 			{
-				impl = settings.CacheProvider.BuildCache(name, properties);
+				ccs.Cache = settings.CacheProvider.BuildCache(name, properties);
 			}
 			catch (CacheException e)


Anyway I do not know of any cache implementation throwing this exception... Those from NHibernate-Caches never throw it.

fredericDelaporte · 2018-06-27T14:18:53Z

src/NHibernate/Cache/ICacheConcurrencyStrategy.cs

@@ -134,13 +135,17 @@ public partial interface ICacheConcurrencyStrategy
 		/// </summary>
 		string RegionName { get; }

+		// 6.0 TODO: type as BaseCache instead
+#pragma warning disable 618


Implementors will be warned by the ICache obsolete message about that, but they will not be able to actually prepare for it. I do not see a way to avoid this. The HashtableCacheProvider.BuildCache implementation pattern commented previously is not usable here, as we may have a cache not deriving from CacheBase here.

fredericDelaporte · 2018-06-27T14:21:48Z

src/NHibernate/Cache/ICacheConcurrencyStrategy.cs

+			// PreferMultipleGet yields false if !IBatchableCacheConcurrencyStrategy, no GetMany call should be done
+			// in such case.
+			return ReflectHelper
+				.CastOrThrow<IBatchableCacheConcurrencyStrategy>(cache, "batching")


This GetMany is currently called only in case of lazy batch fetches (using batch-size). The cases where NHibernate calls GetMany even if PreferMultipleGet is false do not use this GetMany.

fredericDelaporte · 2018-06-27T14:27:15Z

src/NHibernate/Cache/ICacheConcurrencyStrategy.cs

+			var concreteCache = cache.Cache;
+			if (concreteCache == null)
+				return null;
+			return concreteCache as CacheBase ?? new ObsoleteCacheWrapper(concreteCache);


This allows a simplification, by always yielding a CacheBase provided there is a cache, even if it does not implement itself CacheBase.
In most cases it will yield the wrapper held by the cache concurrency, avoiding the instantiation here. (The instantiation will occur only if the user uses a custom cache concurrency, which looks very unlikely to me, since the mapping does not allow specifying a custom implementation.)

fredericDelaporte · 2018-06-27T14:34:18Z

src/NHibernate/Cache/ReadWriteCache.cs

 				try
 				{
-					cache.Lock(key);


The finally is done for unlocking, but there is nothing to unlock if the locking has failed. This line should have been before the try, even without having the lockValue to handle. All similar cases have been changed likewise.

fredericDelaporte · 2018-06-27T14:39:24Z

src/NHibernate/Cache/IBatchableCacheConcurrencyStrategy.cs

 		/// <exception cref="CacheException"></exception>
 		bool[] PutMany(CacheKey[] keys, object[] values, long timestamp, object[] versions, IComparer[] versionComparers,
 								  bool[] minimalPuts);
+
+		// 6.0 TODO: remove for using ICacheConcurrencyStrategy.Cache re-typed CacheBase instead
+		CacheBase CacheBase { get; }


We can change this interface as it is not yet released.

This property is somewhat obsolete as soon as added. But it should not bother many implementors, since mapping does not allow to specify custom cache concurrency implementations.

src/NHibernate/Engine/BatchFetchQueue.cs

fredericDelaporte · 2018-07-02T10:10:31Z

Rebased and squashed.

src/NHibernate/Cache/IBatchableCacheConcurrencyStrategy.cs

src/NHibernate/Cache/ReadOnlyCache.cs

hazzik · 2018-07-07T06:34:46Z

One note: the XXXBase pattern is not found anywhere in the code base. Instead NH is tend to use Java borrowed pattern of AbstractXXX, not sure what I do prefer though.

fredericDelaporte · 2018-07-07T10:29:45Z

One note: the XXXBase pattern is not found anywhere in the code base. Instead NH is tend to use Java borrowed pattern of AbstractXXX, not sure what I do prefer though.

83 matches for class Abstract in NHibernate project, 24 for class [a-zA-Z0-9_]+Base[^a-zA-Z0-9_]. So the Java pattern clearly dominates, but the .Net one is already there for around 20% of cases.

For the cache case, I dislike a bit both patterns indeed. I thought of naming it just Cache. After all DbCommand does not include Base in its name either. But the Db ones are well known classes. The purpose of a Cache class would be less obvious. So I have kept the Base suffix.

fredericDelaporte · 2018-07-13T14:59:58Z

Squashed and rebased for resolving conflicts with #1788.

fredericDelaporte · 2018-10-30T13:10:27Z

Rebased, and one additional adjustment suggested by @hazzik has been made.

As proposed on the development group, and since no continued grounded disagreement has been expressed there since one week after raised concerns being addressed, I intend to merge this PR next week (likely on Wednesday).

(I am issuing this notice on PR issued before starting to apply this handling of merges. I will not do it for new PR.)

fredericDelaporte added t: Improvement c: Core p: Minor labels Jun 27, 2018

fredericDelaporte added this to the 5.2 milestone Jun 27, 2018

hazzik changed the title ~~Replace ICache interface by a BaseCache class~~ Replace ICache interface by a CacheBase class Jun 27, 2018

This comment has been minimized.

Sign in to view

fredericDelaporte commented Jun 27, 2018

View reviewed changes

fredericDelaporte force-pushed the CacheBase branch from bc1aea5 to e3f6bc9 Compare June 27, 2018 14:47

This comment has been minimized.

Sign in to view

maca88 reviewed Jun 27, 2018

View reviewed changes

src/NHibernate/Engine/BatchFetchQueue.cs Show resolved Hide resolved

hazzik reviewed Jun 28, 2018

View reviewed changes

src/NHibernate/Engine/BatchFetchQueue.cs Show resolved Hide resolved

fredericDelaporte force-pushed the CacheBase branch from ab5b465 to 459e840 Compare July 2, 2018 10:10

hazzik reviewed Jul 3, 2018

View reviewed changes

src/NHibernate/Cache/IBatchableCacheConcurrencyStrategy.cs Outdated Show resolved Hide resolved

hazzik reviewed Jul 3, 2018

View reviewed changes

src/NHibernate/Cache/ReadOnlyCache.cs Show resolved Hide resolved

hazzik reviewed Jul 3, 2018

View reviewed changes

src/NHibernate/Cache/ReadOnlyCache.cs Outdated Show resolved Hide resolved

fredericDelaporte force-pushed the CacheBase branch from 7331b4f to 588b121 Compare July 7, 2018 14:30

fredericDelaporte mentioned this pull request Jul 9, 2018

Implement multiple get and put for query cache and query batch #1788

Merged

fredericDelaporte force-pushed the CacheBase branch from 7f9ef68 to e8d9dc6 Compare July 13, 2018 14:54

Replace ICache interface by a CacheBase class

4061f46

fredericDelaporte force-pushed the CacheBase branch from e8d9dc6 to 2a28975 Compare October 30, 2018 13:05

Switch PreferMultipleGet to virtual, and false by default

fb0a3ac

fredericDelaporte force-pushed the CacheBase branch from 2a28975 to fb0a3ac Compare October 30, 2018 13:09

fredericDelaporte mentioned this pull request Oct 30, 2018

Fix cache build for honoring mapped concurrency #1480

Merged

hazzik approved these changes Oct 30, 2018

View reviewed changes

fredericDelaporte merged commit 6fedcb6 into nhibernate:master Oct 31, 2018

fredericDelaporte deleted the CacheBase branch October 31, 2018 16:36

fredericDelaporte added the r: Fixed label Oct 31, 2018

fredericDelaporte mentioned this pull request Nov 27, 2018

Upgrade to NH 5.2.0 nhibernate/NHibernate-Caches#56

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ICache interface by a CacheBase class #1777

Replace ICache interface by a CacheBase class #1777

fredericDelaporte commented Jun 27, 2018

This comment has been minimized.

fredericDelaporte Jun 27, 2018

fredericDelaporte Jun 27, 2018

fredericDelaporte Jun 27, 2018

fredericDelaporte Jun 27, 2018 •

edited

Loading

fredericDelaporte Jun 27, 2018

fredericDelaporte Jun 27, 2018

fredericDelaporte Jun 27, 2018

fredericDelaporte Jun 27, 2018

fredericDelaporte Jun 27, 2018

This comment has been minimized.

fredericDelaporte commented Jul 2, 2018

hazzik commented Jul 7, 2018 •

edited

Loading

fredericDelaporte commented Jul 7, 2018

fredericDelaporte commented Jul 13, 2018

fredericDelaporte commented Oct 30, 2018

Replace ICache interface by a CacheBase class #1777

Replace ICache interface by a CacheBase class #1777

Conversation

fredericDelaporte commented Jun 27, 2018

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

fredericDelaporte commented Jul 2, 2018

hazzik commented Jul 7, 2018 • edited Loading

fredericDelaporte commented Jul 7, 2018

fredericDelaporte commented Jul 13, 2018

fredericDelaporte commented Oct 30, 2018

fredericDelaporte Jun 27, 2018 •

edited

Loading

hazzik commented Jul 7, 2018 •

edited

Loading