-
Notifications
You must be signed in to change notification settings - Fork 924
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
UpdateTimestampsCache non cached entities concurrency issue #2735
Comments
The simplest solution would be to remove the locking entierly from |
I agree @maca88 best solution remove locking entierly from Why an entity dml blocks another entity read, it's not occurred when we are using And you are right our application is running on openshift, 25 application (minimum 2 pods every app) rely on same local distributed cache. UpdateTimestampsCache is important for clients to detect stale data but not important as locking whole application. |
@maca88 I am willing to implement this feature if you can give a little guidance.
What do you think about the offer and how should we proceed, which branch should I use as a base? |
master. |
Updating timestamps for only cached entities is not a valid approach. Consider this query:
where In short, with your |
@maca88 When we use redis pub/sub distributed cache in your implementation non cached entities causes too much synchronization events. Think about a configuration in entity mapping If this approach is ok I could implement this in NHibernate and FluentNHibernate libraries. There is also locks in |
The
How would
In order to further improve the
By synchronizing the local cache data every X seconds, you gain performance at a cost of potentially having stale data that is up to X seconds old. |
Current Entity Mapping Strategies namespace FluentNHibernate.Mapping
{
public class CachePart : ICacheMappingProvider
{
public CachePart(Type entityType);
public CachePart CustomInclude(string custom);
public CachePart CustomUsage(string custom);
public CachePart IncludeAll();
public CachePart IncludeNonLazy();
public CachePart NonStrictReadWrite();
public CachePart ReadOnly();
public CachePart ReadWrite();
public CachePart Region(string name);
public CachePart Transactional();
}
} I thought that we could add new strategy As its a new feature so it would not be a breaking change. Don't get me wrong removing lock at UpdateTimestampsCache(#2742) is a good start point, as you mentioned I could implement in custom region strategy. Just trying to improve NHibernate if it's make sense. |
I am fine with that, feel free to create a PR for it. |
But UpdateTimestampsCache and entity cachability are still orthogonal things and shouldn't be connected. Being able to easily circumvent the behavior with a custom UpdateTimestampsCache implementation seems like a feasible solution. |
I disagree. SetCacheable shouldn't be the cause an exception, IMO. It's a hint, not a request. Having Cache.Never as a setting to prevent certain entities (or columns/properties) from ever entering a cache could be useful. |
Yes, but I'm not sure throwing an exception is a good thing. It should just deem the query uncacheable, perhaps log it, and move on. IMO, SetCacheable just means "cache it if you can". Nothing else. It doesn't throw an exception if query caching is entirely disabled. |
IMO such behavior can lead to unexpected hard to notice performance issues. For me it's better to throw and make user mark affected queries not cacheable explicitly or remove |
I do agree, we should probably move the configuration to a separate option that could be configured on the entities and collections tables. For example:
this way an entity can still be cached with the defined cache strategy, but cannot be used in a cached query.
By just replacing the
For me both approaches are valid. In this case I am leaning more towards using the fail-fast principle rather than fail-silently. I tend to agree with @bahusoid that it is not the same as globally enable/disable the cache. It is nice that we can globally enable/disable the cache based on a specific environment without modifying the code, but it is not nice to have a query that will never behave the way we defined it. |
I understand that sentiment, but coming from a scenario where query composition is very common, the "query" in question is not defined up front, and may well include entities or properties that could have the proposed "Never" setting. I see a possible use of the Never setting as a security thing, i.e "this value should never be allowed to be stored in the cache layer". Separating query cache settings from entity settings seems like a very good idea, and possibly it could be something that is bound to properties, rather that classes. Specifying it on class level would just be a convenience thing, to set a default. While there's certainly such a thing as too many configuration settings, perhaps the "to throw or not to throw an exception" thing could warrant a setting. But I still say that SetCacheable is only a hint. |
I do agree that it would be nice to have a setting bound to properties, but I think this is a separate issue and outside the scope of this issue. For example the following query:
will put only the
For this feature I think we should use a separate setting:
in the above example |
Main issue is in real life scenario most of entities does not use cache and we should give a configuration that entity never uses cache so don’t track UpdateTimeStamps. I disagree seperate configuration on entity level, it could lead conflicts and again main issue is tracking UpdateTimestampsCache on never cached entities. |
And better configuration options is a way to solve that. Given the current setup, using UpdateTimestampsCache on never cached entities is the only correct thing to do. |
There is open pr for this issue and I am willing to improve, could you explain how to resolve concurrency issue on never cached entities? |
It is. It's used for the query cache. |
@hazzik you have a pull request #2130 adressing this issue that I have been reported 2 years ago at 2019. @maca88 @bahusoid What dou you think, how dou we proceed. If proposal not accepted, after removing lock in UpdateTimestampsCache I could move non cached entity logic into UpdateTimestamps region cache custom implementation. Not preferred but at least it could be customized on our side. There is one Bottleneck Timestamper.Next() which has lock in it, but it could be ignored. I also think this lock could be entity persister level. public static long Next()
{
lock (lockObject)
{
// Ticks is accurate down to 100 nanoseconds - hibernate uses milliseconds
// to help calculate next time so drop the nanoseconds portion.(1ms==1000000ns)
long newTime = ((DateTime.UtcNow.Ticks / 10000) - baseDateMs) << BinDigits;
if (time < newTime)
{
time = newTime;
counter = 0;
}
else if (counter < OneMs - 1)
{
counter++;
}
return time + counter;
}
} |
I'm fine with both original plan (
@gliljas I disagree. Ability to globally enable/disable cache doesn't make it a hint. When cache is enabled
@gliljas So suggested PR seems already implements this use case - disables entity caching and forbids usages in other cache layers (query cache) |
@bahusoid |
JPA just uses word hint for any query customization option. It has nothing to do with "do it if you can" hint meaning (at least from hibernate standpoint) You can check other hibernate "hints" to see it. Hibernate just provides |
By looking at the Hibernate code, it seems like that As I mentioned before, I am also fine that it is implemented as a hint, but at least we should log a warning so that the developer could see that the query won't be cached. We could optionally add a setting for throwing an exception instead.
query cache and cache strategy on an entity/collection level are two separate things so I don't see how it would lead to conflicts. From an end user standpoint, I do understand that |
@maca88 Again as I already said all those "hints" come from JPA (Java Persistence API). JPA uses this term for query configuration options specific for JPA implementations (and hibernates implements JPA) So yes it's a hint in a way that if NHIbernate doesn't support provided hint via JPA - it will be ignored (though didn't check it) . But it doesn't mean that supported by NHIbernate hints (query options like readonly, comment, timeout, cacheable ... ) should be considered optional when applied to query. Also check Hibernate docs (see examples "Caching query using JPA" and "Caching query using Hibernate native API") - word hint is used only for JPA examples. It seems my point is clear here - I vote for throwing vs silent behavior change (I see it as a potential source of hard to identify bugs and perf issues) . No more noise from me on this one - do whatever you decide is better. |
From end user perspective I think this could lead conflict, when we talk about second level cache if its enabled then you can use query cache or entity cache. If we change configuration as you suggest there would be too much configuration.
When Query Cache true and Entity Cache is not configured somethink, then just unique keys are stored in QueryCache, always gets entity from database when requested. On the other hand I think Cache Never has a simpler approach? I also vote for throwing exception if its counts :) |
Obsoleted by #2744, which allows to exclude entities from all second level cache features. |
Hi @hazzik @maca88,
As I mentioned earlier current UpdateTimestampsCache has still concurrency issue. #2129
In real life scenario some of entities uses second level cache but most of entities does not uses second level cache but still try to update UpdateTimestampsCache. At least it should be configurable in entity level or session factory level, default may be true for backward compability issue.
Non cached entities could be related(foreign key) with cached entity may be better solution StandardQueryCache property spaces and cached entity property spaces only triggers UpdateTimestampsCache?
Current UpdateTimestampsCache uses AsyncReaderWriterLock if one writer grants lock other writers of different entities or readers wait for it. If this is true you could realize the concurrency issue that we are facing under high concurrency (400-500tps).
If still not acceptable please consider a way to implement Custom UpdateTimestampsCache via UpdateTimestampsFactory like cache.provider_class attribute in configuration. Could be named cache.update_timestamps_class.
Currently we are trying to replace current UpdateTimestampsCache via reflection but its not cleanest solution.
The text was updated successfully, but these errors were encountered: