Add support for query space synchronization #1392

gliljas · 2017-10-18T21:02:31Z

Ok, this was a bit of a Pandora's box...
Fixes #1381

The problem was that using an ISQLQuery and ExecuteUpdate causes the entire (sort of) 2nd level cache to be cleared. Not a problem with an in-memory cache provider, but with a distributed cache provider (Redis in our case), it's a massive problem. We ended up having to create ugly wrappers, with native SqlCommand etc.

I found this...

https://stackoverflow.com/questions/19054673/2nd-level-cache-invalidation-with-native-queries

...which indicated that Hibernate had a solution for this, but these methods were missing in NHibernate.

The first thing to do was a unit test, to verify the behavior. This is the first assert in
NativeSQLBulkOperationsWithCache.cs .

My expectation was that it would call the ICache.Clear method once (only one queryspace/table in the mapping), but found that it called the method 10 times (!). That's quite costly in a distributed scenario, especially if you have more than one mapped class. We do. :)

The reasons for this are two.

There's a method in NativeSQLQueryPlan.cs named CoordinateSharedCacheCleanup, called from PerformExecuteUpdate, which creates a BulkOperationCleanupAction and calls Init() on it. This call causes a first wave of ICache.Clear:s. I don't understand why a native ExecuteUpdate would need to clear the cache before it's run, but it does. The Hibernate version of this looks about the same, but never calls Init(), so the cache is never touched. I haven't touched this yet, since I wanted your input, but it seems like the Init call should be removed.
BulkOperationCleanupAction calls EvictEntity for every queryspace it finds, but this may very well end up with the same IPersister many times, and the same ICache just as many times.

Since the unit test has a mapped base class, subclassed into 4, we ended up with 2*5 calls to the Vehicle cache.

This was fixed by adding an overload of EvictEntity (and its siblings) which takes an IEnumerable<string> (entity names), which can be handled as a batch, calling each ICache only once. Async versions were also added. Please take a look at the Task.WhenAny(Task.WhenAll....) construct, which is used to exit via cancellation. It may be worth discussing.

The AddSynchronizedQuerySpace method was added via an ISynchronizableQuery interface, just as Hibernate uses SynchronizeableQuery (notice the spelling, which I think is wrong). Java does things with its generics which I don't think is possible in C#, but I replicated the behavior using a self referencing generic interface. It's somewhat elegant, but also a bit weird. Just using a plain ISynchronizableQuery interface would prevent method chaining, but an option could be to simply add the methods to ISQLQuery.

I replicated the Hibernate unit test I found for this, which checks whether the session is flushed unnecessarily.

As I mentioned on the developer list, I've added NSubstitute to the project.

For the ICacheProvider test it wasn't super elegant, but that's also an indication that things in the core code is a bit coupled.
The BulkOperationCleanupAction tests were a much better fit, and such tests could be used a lot more.
NSubstitute 3 (I'm using v2 in my own projects) apparently has a dependency on NETStandard. It adds a boat load of subdependencies, but it seems to work just fine, after fixing a couple of ambiguous names. If it causes problems, v2 works fine too.

fredericDelaporte · 2017-10-19T00:33:26Z

It may be 5.1 if it has no breaking changes, otherwise 6.0. Currently I am focusing on 5.0.1 and also 5.0 releases of some dependent projects like caches.

gliljas · 2017-10-19T11:24:08Z

5.1 would be great, since it's a performance drag. As far as breaking changes go, it adds new methods to public interfaces (ISQLQuery, ISessionFactory), but some of these could be hidden/internalized in a first release.

hazzik · 2017-11-17T04:16:17Z

src/NHibernate/Impl/SqlQueryImpl.cs

@@ -170,10 +172,12 @@ public override IList<T> List<T>()

 		public NativeSQLQuerySpecification GenerateQuerySpecification(IDictionary<string, TypedValue> parameters)
 		{
+			var allQuerySpaces=new HashSet<string>(querySpaces ?? new List<string>());
+			allQuerySpaces.UnionWith(GetSynchronizedQuerySpaces());


NativeSQLQuerySpecification already does deduplication, so we dont' need to do it here.

fredericDelaporte

it seems like the Init call should be removed.

Agreed. Init does only do the cache clearing, which should be done after query execution.
I think CoordinateSharedCacheCleanup calls it because it is a duplicate of the one in AbstractStatementExecutor, for which it does maybe make sens to clear the cache before hand.
Or doing it before was maybe a crutch for avoiding returning stale data for concurrent sessions right after the SQL update before the end of ExecuteUpdate had cleared the cache. But such a logic does not prevent a concurrent session to put again things in the cache before the sql update have done its job, so if this the reason, it was a bit moot. So anyway I think this Init call should be removed in the case of the CoordinateSharedCacheCleanup method of NativeSQLQueryPlan.

As far as breaking changes go, it adds new methods to public interfaces (ISQLQuery, ISessionFactory), but some of these could be hidden/internalized in a first release.

It also add a new ancestor to an interface, which is also a breaking change, strictly speaking.

So either it is possible to change this PR in a way where nothing changes on existing interfaces (extension methods with likely hard cast to implementation in them, do nothing if not the expected implementation...), or we have to bump next version to 6.0 instead of 5.1 if merging this.

Java does things with its generics which I don't think is possible in C#

It looks like the return type covariance I had to deal with for session builders, see this class and interface. (In this case I could additionally "hide" the generic type (class/interface eight line above) but I do not think this can apply to query space case.)

fredericDelaporte · 2017-11-22T18:03:21Z

src/NHibernate/Impl/AbstractQueryImpl.cs

@@ -42,7 +42,7 @@ public abstract partial class AbstractQueryImpl : IQuery
 		private CacheMode? cacheMode;
 		private CacheMode? sessionCacheMode;
 		private string comment;
-
+		


Accidental white-space addition.

fredericDelaporte · 2017-11-23T15:27:38Z

The BulkOperationCleanupAction has quite changed on Hibernate side. The Init method is gone. That was done in HHH-4034 commit.

The Init removal is completed by an else for ensuring the cleanup is done, see here. We should likely do the same, for the native sql case and for the abstract executor case too (see here).

gliljas · 2018-01-22T21:07:11Z

@fredericDelaporte Any suggestion on how to make this suitable for inclusion?

hazzik · 2018-01-23T03:40:11Z

@gliljas at least resolve the conflicts and made the build green.

Fixed redundant cache clearing

Return task as cancelled

hazzik · 2018-04-16T08:09:04Z

src/NHibernate/Impl/SessionFactoryImpl.cs

+
+				if (log.IsDebugEnabled())
+				{
+					foreach (var p in cacheGroup)


I would love to see only one line in logs in this case.

hazzik · 2018-04-16T08:09:11Z

src/NHibernate/Impl/SessionFactoryImpl.cs

+				{
+					foreach (var p in cacheGroup)
+					{
+						log.Debug("evicting second-level cache: " + p.EntityName);


I would love to see only one line in logs in this case.

hazzik · 2018-04-16T08:24:48Z

src/NHibernate/Impl/SqlQueryImpl.cs

+
+		public IReadOnlyCollection<string> GetSynchronizedQuerySpaces()
+		{
+			return addedQuerySpaces.IsValueCreated ? addedQuerySpaces.Value : new HashSet<string>();


The new HashSet<string>() kills the purpose of the Lazy<> here.

I think the Lazy<> is not worth it here. For having it useful, GetSynchronizedQuerySpaces() would have to yield null instead when it is not initialized. Would it be a private method, why not, but as an interface method, better yield an empty collection.

To be squashed.

gliljas · 2018-06-04T18:44:07Z

I ran the build locally on WSL, and I think I managed to fix it by updating NSubstitute to 3.1.0.

fredericDelaporte · 2018-06-05T10:22:23Z

The Travis build is fixed. I had thought about upgrading NSubstitue, but looking at the references of both versions, I was having the impression it will not change a thing.

fredericDelaporte · 2018-06-05T10:57:23Z

I have run an async regen just to verify the tests due to the async upgrade and configuration change on master, which may impact them.
Looking at them, nothing should have changed. But in fact a change was missing in NHibernate code instead, I do not have analyzed why yet.

hazzik · 2018-06-05T11:04:50Z

src/NHibernate/Async/Action/BulkOperationCleanupAction.cs

 			}
 		}

 		#endregion
-
-		public virtual async Task InitAsync(CancellationToken cancellationToken)


"Since the class is not internal, this is a breaking change. It needs to be added back, obsoleted, in the non-generated partial."

fredericDelaporte · 2018-06-05T11:15:53Z

src/NHibernate/Async/Hql/Ast/ANTLR/Exec/AbstractStatementExecutor.cs

@@ -34,19 +34,6 @@ public abstract partial class AbstractStatementExecutor : IStatementExecutor

 		public abstract Task<int> ExecuteAsync(QueryParameters parameters, ISessionImplementor session, CancellationToken cancellationToken);

-		protected virtual async Task CoordinateSharedCacheCleanupAsync(ISessionImplementor session, CancellationToken cancellationToken)


Yes, and same for this one.

hazzik · 2018-06-08T11:32:54Z

src/NHibernate/ISQLQuery.cs

+		/// <returns>The query.</returns>
+		public static ISQLQuery AddSynchronizedQuerySpace(this ISQLQuery sqlQuery, string querySpace)
+		{
+			if (!(sqlQuery is ISynchronizableSQLQuery ssq))


@fredericDelaporte I think then it shall be CastOrThrow

hazzik · 2018-06-08T11:34:36Z

src/NHibernate/Hql/Ast/ANTLR/Exec/AbstractStatementExecutor.cs

+
+		// Since v5.2
+		[Obsolete("This method has no more actual async calls to do, use its sync version instead.")]
+		protected virtual Task CoordinateSharedCacheCleanupAsync(ISessionImplementor session, CancellationToken cancellationToken)


Need either async keyword, or wrapping into try-catch.

hazzik · 2018-06-08T11:45:17Z

src/NHibernate/ISynchronizableQuery.cs

+
+namespace NHibernate
+{
+	public interface ISynchronizableQuery<out T> where T : ISynchronizableQuery<T>


Do we plan to have other types of ISynchronizableQuery<> other than ISQLQuery?

Looking at Hibernate code, it seems not. They use it for SQLQuery (obsoleted on their side) and NativeQuery (replacement for SQLQuery). So why not merging that in ISynchronizableSQLQuery without the generic param trick.

To be squashed.

Query space invalidation is not available in 5.1.x, see nhibernate#1392. To be squashed

gliljas requested a review from fredericDelaporte October 18, 2017 21:02

gliljas closed this Oct 19, 2017

gliljas reopened this Oct 19, 2017

hazzik added the t: New Feature label Oct 24, 2017

hazzik changed the title ~~GH-1381: Added support for query space synchronization~~ Add support for query space synchronization Oct 27, 2017

hazzik reviewed Nov 17, 2017

View reviewed changes

fredericDelaporte reviewed Nov 22, 2017

View reviewed changes

gliljas mentioned this pull request Nov 23, 2017

Async After-/BeforeTransactionCompletion #1452

Merged

gliljas added 5 commits January 25, 2018 17:39

Added support for query space synchronization

382e5d3

Fixed redundant cache clearing

Method documentation on ISynchronizableQuery

f0043c5

Return task as cancelled

Removed call to BulkOperationCleanupAction.Init()

2f11b74

Removed the Init method

588e5ad

Resolved conflicts and corrected from review

ecf3e89

gliljas force-pushed the GH-1381 branch from 57eb347 to ecf3e89 Compare January 25, 2018 18:26

fredericDelaporte added this to the 5.2 milestone Mar 26, 2018

hazzik reviewed Apr 16, 2018

View reviewed changes

Merge branch 'master' into nhibernateGH-1381

79d6362

hazzik reviewed Apr 16, 2018

View reviewed changes

hazzik mentioned this pull request May 7, 2018

Added support for batching 2nd level cache operations when loading entities and collections #1633

Merged

fredericDelaporte added t: Fix and removed t: New Feature labels May 9, 2018

fredericDelaporte and others added 3 commits May 9, 2018 16:42

Merge branch 'master' into nhibernateGH-1381

9b27dd8

Adjust code according to reviews

e10cb9f

To be squashed.

Remove breaking changes

e96fb59

To be squashed.

gliljas added 2 commits June 4, 2018 19:47

Update NSubstitute to 3.1.0

bf14184

Merge branch 'master' into nhibernateGH-1381

223412c

fredericDelaporte previously approved these changes Jun 5, 2018

View reviewed changes

hazzik self-requested a review June 5, 2018 10:42

hazzik previously approved these changes Jun 5, 2018

View reviewed changes

Async regen.

c7f2968

fredericDelaporte dismissed stale reviews from hazzik and themself via c7f2968 June 5, 2018 10:56

hazzik reviewed Jun 5, 2018

View reviewed changes

fredericDelaporte reviewed Jun 5, 2018

View reviewed changes

Gunnar Liljas and others added 2 commits June 7, 2018 18:59

Readded async methods as Obsolete

641a29f

Add obsolete message and version

648f6c4

fredericDelaporte previously approved these changes Jun 7, 2018

View reviewed changes

hazzik reviewed Jun 8, 2018

View reviewed changes

Address review

ac9a445

To be squashed.

fredericDelaporte dismissed their stale review via ac9a445 June 8, 2018 13:00

hazzik and others added 2 commits June 9, 2018 02:06

Merge branch 'master' into nhibernateGH-1381

2640ce1

Merge branch 'master' into nhibernateGH-1381

c5afe02

fredericDelaporte approved these changes Jun 12, 2018

View reviewed changes

Merge branch 'master' into nhibernateGH-1381

690b6d3

fredericDelaporte merged commit 1024b71 into nhibernate:master Jun 13, 2018

fredericDelaporte added the r: Fixed label Jun 13, 2018

gliljas deleted the GH-1381 branch October 29, 2018 17:09

fredericDelaporte added a commit to fredericDelaporte/nhibernate-core that referenced this pull request Jan 6, 2019

Adapt tests to lack of query space invalidation

e986308

Query space invalidation is not available in 5.1.x, see nhibernate#1392. To be squashed

fredericDelaporte mentioned this pull request Jan 6, 2019

Backport Query space invalidation doesn't work for bulk actions #1959

Merged

		@@ -34,19 +34,6 @@ public abstract partial class AbstractStatementExecutor : IStatementExecutor

		public abstract Task<int> ExecuteAsync(QueryParameters parameters, ISessionImplementor session, CancellationToken cancellationToken);

		protected virtual async Task CoordinateSharedCacheCleanupAsync(ISessionImplementor session, CancellationToken cancellationToken)

Add support for query space synchronization #1392

Add support for query space synchronization #1392

Uh oh!

Conversation

gliljas commented Oct 18, 2017 • edited by fredericDelaporte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Oct 19, 2017

Uh oh!

gliljas commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Nov 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gliljas commented Jan 22, 2018

Uh oh!

hazzik commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gliljas commented Jun 4, 2018

Uh oh!

fredericDelaporte commented Jun 5, 2018

Uh oh!

fredericDelaporte commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gliljas commented Oct 18, 2017 •

edited by fredericDelaporte

Loading

fredericDelaporte commented Nov 23, 2017 •

edited

Loading

fredericDelaporte commented Jun 5, 2018 •

edited

Loading

hazzik Jun 8, 2018 •

edited

Loading