-
Notifications
You must be signed in to change notification settings - Fork 934
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
Fix cache build for honoring mapped concurrency #1480
Conversation
17541fc
to
a086ade
Compare
I think this refactoring went a wrong way. (But I'm not sure what is the right 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? |
Didn't know such a word exists.
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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8c0e9ec
to
7bb6d1a
Compare
3084f64
to
511ef9e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e00f702
to
ebde9af
Compare
This comment has been minimized.
This comment has been minimized.
ebde9af
to
b900708
Compare
b900708
to
277ca6a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
537a1c0
to
e377772
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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. |
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 will address the life-cycle trouble.
Thanks for your changes. After having moved the destroy handling, LGTM. |
Added back from the old original commit which was later squashed with a commit giving-up this uniqueness.
eb840bd
to
fcd00a9
Compare
Rebased, and obsolete comments version updated. |
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.
LGTM
This PR is a follow-up to #1476. It aims at fixing:
Possible breaking change:
ICache
caches yielded by the session factory will be aCacheBase
wrapper around the cache actually provided by the cache provider, if it was not deriving fromCacheBase
.