Skip to content

WIP - Do not try to invalidate query spaces for non cached persisters - #2130

Draft
hazzik wants to merge 4 commits into
nhibernate:masterfrom
hazzik:gh-2129
Draft

hazzik wants to merge 4 commits into
nhibernate:masterfrom
hazzik:gh-2129

Conversation

@hazzik

@hazzik hazzik commented Apr 15, 2019

Copy link
Copy Markdown
Member

Fixes #2129

The second commit is a code cleanup. It could be dropped if serialization changes are not desired.

@hazzik hazzik added the t: Fix label Apr 15, 2019
Comment thread src/NHibernate/Action/BulkOperationCleanupAction.cs Outdated
Comment thread src/NHibernate/Action/BulkOperationCleanupAction.cs Outdated
Comment thread src/NHibernate/Action/BulkOperationCleanupAction.cs Outdated
Comment thread src/NHibernate/Action/BulkOperationCleanupAction.cs Outdated
}
ISet<string> roles = session.Factory.GetCollectionRolesByEntityParticipant(affectedQueryables[i].EntityName);

var roles = _factory.GetCollectionRolesByEntityParticipant(queryables.EntityName);

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.

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?

@gliljas

gliljas commented Apr 30, 2019

Copy link
Copy Markdown
Member

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.

@hazzik hazzik changed the title Do not try to invalidate query spaces for non cached entities Do not try to invalidate query spaces for non cached persisters Apr 30, 2019
@hazzik

hazzik commented Apr 30, 2019

Copy link
Copy Markdown
Member Author

Probably you're right. I need to check.

@hazzik hazzik changed the title Do not try to invalidate query spaces for non cached persisters WIP - Do not try to invalidate query spaces for non cached persisters Apr 30, 2019
@hazzik

hazzik commented May 17, 2020

Copy link
Copy Markdown
Member Author

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.

@gliljas are you saying that an entity could be cached in a query even when the entity is not cacheable?

@fredericDelaporte

Copy link
Copy Markdown
Member

Not the entity, but its identifiers could, as the query cache caches identifiers only, regardless of entity cacheability.

@gliljas

gliljas commented Jun 1, 2020

Copy link
Copy Markdown
Member

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.

@gliljas

gliljas commented Jun 1, 2020

Copy link
Copy Markdown
Member

A working feature might be to only allow queries to be cached if all included queryspaces/persisters have caching enabled.

@hazzik

hazzik commented Jun 1, 2020

Copy link
Copy Markdown
Member Author

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.

@fredericDelaporte

Copy link
Copy Markdown
Member

A working feature might be to only allow queries to be cached if all included queryspaces/persisters have caching enabled.

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.

@gokhanabatay

Copy link
Copy Markdown
Contributor

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.

@hazzik @fredericDelaporte

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prevent calling UpdateTimesStampsCache for non cached entites

5 participants