Skip to content

Fix cache build for honoring mapped concurrency - #1480

Merged
fredericDelaporte merged 11 commits into
nhibernate:masterfrom
fredericDelaporte:CacheBuild
Feb 24, 2019
Merged

fredericDelaporte merged 11 commits into
nhibernate:masterfrom
fredericDelaporte:CacheBuild

Conversation

@fredericDelaporte

@fredericDelaporte fredericDelaporte commented Dec 9, 2017

Copy link
Copy Markdown
Member

This PR is a follow-up to #1476. It aims at fixing:

  • The resolved concurrency strategy. Currently, when two entities share the same region but have a different concurrency strategy, the first having its cache built will "force" the other to use the same concurrency. (This trouble was not occurring for collections caches.)
  • The duplication of cache concurrency strategies: currently each cached collection recreate its cache concurrency strategy without checking if one has already been built for its region. (Entities do check instead.)

Possible breaking change: ICache caches yielded by the session factory will be a CacheBase wrapper around the cache actually provided by the cache provider, if it was not deriving from CacheBase.

Comment thread src/NHibernate/Cache/CacheFactory.cs Outdated
Comment thread src/NHibernate/Cache/CacheFactory.cs
Comment thread src/NHibernate/Impl/SessionFactoryImpl.cs
@hazzik
hazzik self-requested a review December 14, 2017 22:24
@hazzik

hazzik commented Dec 14, 2017

Copy link
Copy Markdown
Member

I think this refactoring went a wrong way. (But I'm not sure what is the right way).

@fredericDelaporte

Copy link
Copy Markdown
Member Author

I think this refactoring went a wrong way.

It is not just a refactoring, it is a bug fix. We currently have bugs, like resolved entity concurrency not matching the one put in mappings.

A refactoring could be extracting the cache handling out of the session factory, as Hibernate has done (but without fixing the bug mentioned here). But their refactoring is not easily portable to NHibernate, because it looks heavily coupled and even duplicated in their many cache providers (ehCache, singleton ehCache, infiniSpan, ...).

Can you explain what seems wronger in this PR than current implementation in master? I do not think this solution is perfect, but I think it is more correct. If there are troublesome points in it, it is of course always worth pointing them for checking if we can do better.

But after that, even if we find no way of getting those points better, we should then check if overall we agree this PR is still be better than current implementation. And if yes then we should accept it, don't you think?

@hazzik

hazzik commented Dec 15, 2017

Copy link
Copy Markdown
Member

wronger

Didn't know such a word exists.

Can you explain what seems wronger in this PR than current implementation in master?

I did not say that this is worse than the master, I just feel that this is a wrong way, but don't yet know why. I'll take another look later this week.

@hazzik

This comment has been minimized.

@fredericDelaporte

This comment has been minimized.

@fredericDelaporte fredericDelaporte changed the title Fix cache build for honoring mapped concurrency WIP Fix cache build for honoring mapped concurrency Feb 26, 2018
Comment thread src/NHibernate/Cache/CacheFactory.cs
Comment thread src/NHibernate/Impl/SessionFactoryImpl.cs
@fredericDelaporte fredericDelaporte changed the title WIP Fix cache build for honoring mapped concurrency Fix cache build for honoring mapped concurrency Mar 1, 2018
@fredericDelaporte

This comment has been minimized.

@fredericDelaporte

This comment has been minimized.

@fredericDelaporte

This comment has been minimized.

@fredericDelaporte

This comment has been minimized.

@fredericDelaporte

This comment has been minimized.

@fredericDelaporte
fredericDelaporte force-pushed the CacheBuild branch 3 times, most recently from 537a1c0 to e377772 Compare November 6, 2018 16:32
@fredericDelaporte

This comment has been minimized.

@fredericDelaporte

This comment has been minimized.

@fredericDelaporte fredericDelaporte modified the milestones: 5.2, 5.3 Nov 7, 2018
@hazzik

hazzik commented Nov 23, 2018

Copy link
Copy Markdown
Member

I've added some improvements and also I've removed some of the logic which I think unnecessary. You can drop it if you do not like.

Comment thread src/NHibernate/Cache/StandardQueryCache.cs Outdated

@fredericDelaporte fredericDelaporte left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will address the life-cycle trouble.

Comment thread src/NHibernate/Impl/SessionFactoryImpl.cs Outdated
Comment thread src/NHibernate/Impl/SessionFactoryImpl.cs Outdated
@fredericDelaporte

Copy link
Copy Markdown
Member Author

Thanks for your changes. After having moved the destroy handling, LGTM.

@fredericDelaporte

Copy link
Copy Markdown
Member Author

Rebased, and obsolete comments version updated.

@hazzik hazzik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@fredericDelaporte
fredericDelaporte merged commit da429f7 into nhibernate:master Feb 24, 2019
@fredericDelaporte
fredericDelaporte deleted the CacheBuild branch February 24, 2019 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants