Skip to content

Fix cache build for honoring mapped concurrency #1480

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

Merged
merged 11 commits into from
Feb 24, 2019

Conversation

fredericDelaporte
Copy link
Member

@fredericDelaporte fredericDelaporte commented Dec 9, 2017

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.

@hazzik hazzik self-requested a review December 14, 2017 22:24
@hazzik
Copy link
Member

hazzik commented Dec 14, 2017

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

@fredericDelaporte
Copy link
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
Copy link
Member

hazzik commented Dec 15, 2017

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
@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
Copy link
Member

hazzik commented Nov 23, 2018

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.

Copy link
Member Author

@fredericDelaporte fredericDelaporte left a comment

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.

@fredericDelaporte
Copy link
Member Author

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

@fredericDelaporte
Copy link
Member Author

Rebased, and obsolete comments version updated.

Copy link
Member

@hazzik hazzik left a comment

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