-
Notifications
You must be signed in to change notification settings - Fork 934
WIP - Do not try to invalidate query spaces for non cached persisters #2130
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
base: master
Are you sure you want to change the base?
Conversation
} | ||
ISet<string> roles = session.Factory.GetCollectionRolesByEntityParticipant(affectedQueryables[i].EntityName); | ||
|
||
var roles = _factory.GetCollectionRolesByEntityParticipant(queryables.EntityName); |
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.
Collection persister can also have cache. But currently _hasCache
is only calculated for entity persister. And shouldn't we retrieve here list for only cached collection roles?
Hi! Maybe I'm missing something here, and is just focusing on the title, but query space invalidation / time stamp updating is orthogonal to the cachability of entities. |
Probably you're right. I need to check. |
@gliljas are you saying that an entity could be cached in a query even when the entity is not cacheable? |
Not the entity, but its identifiers could, as the query cache caches identifiers only, regardless of entity cacheability. |
Exactly. A cached query stores identifiers (or value projections) and should be invalidated whenever one of the included queryspaces is invalidated. It's not particularly efficient, especially with distributed caches, but it's how works right now. |
A working feature might be to only allow queries to be cached if all included queryspaces/persisters have caching enabled. |
I'll extract some refactoring from this PR into another, because it is useful anyway. For this PR I/we would need to add tests to demonstrate the behavior. |
I do not agree. Query caching is enabled on a query per query basis, by user code. I think we should trust the user knows what he does in such case, instead of disregarding his explicit instructions. |
With NH 5.3 issue still exists As a workaround with reflection we are changing NH UpdateTimestampsCache with our timestamp cache impl. that invalidates only cached entities. There is a huge performance effect under concurrent request of dml operations. |
Fixes #2129
The second commit is a code cleanup. It could be dropped if serialization changes are not desired.