Guenther, first of all I'd like to thank you for all the effort you have put in testing Hibernate and Infinispan. This is really valuable information that helps the community get better software out . Keep it coming!
This is clearly a bug. I've just raised: HHH-4757
Surely the sync block is a bit heavy-handed? I know this is more a suggestion for the Hibernate Timestamper class, but wouldn't the use of an AtomicLong with getAndIncrement() be better?
Avoiding sync block where not strictly neccessary, is surely a good thing.
I don't know well the java.util.concurrent.atomic package, it's new for me, but it sounds good.
Not worth looking at what ehcache calls because that solution would only work on a single node environment.
It works but it's a solution that only works for a single node environment, which means that is not a valid solution .
I have rejected HHH-4757 because in the grand scheme of things, it's better to have a tiny window of 100ms during which queries would not be considered up to date, rather than imposing synchronous communication across the cluster and using more precise next timestamp.
It's a matter of deciding which is the lesser of the two evils. I'd rather see queries considered out of date when they're not, rather than queries considered up to date when they're only partially constructed. The former leads to not-so-efficient behaivour whereas the latter leads to incorrect behaviour.
I am agree to what you said, but after reviewing the thing I saw that the thing works even worser than I originally described :
whenever Hiberante put's a query into cache, it puts as query timestamp the sessions initial timestamp (see HHH-4551).
This leads to following scenario using Infinispan:
1. create new entities on class A
2. commit (UpdateTimestamps puts fresh timestamp for space A: 12345566)
3. start a new Session (as usually happening within next 100 ms, session timestamp is 12345566 too)
4. execute Query, result set put into cache with timestamp 12345566)
5. reexecute Query expecting it's hitting the cache,
indeed it is:
- producing a miss, as isUpToDate returns false (12345566 > 12345566 == false)
- putting again the fresh obtained result-set into cache (again with timestamp 12345566)
Point 5 can be repeated endless times, never producing a hit in the query cache (unless you start a new session).
The only way to get ahead to this problem, is waiting at least 100ms after each timestamp-invalidating commit before opening a new session. But this is very ugly workaround.
I did already propose an inprovement in this way to Hiberante 2 month ago, see
but nobody considers it
After all, in my opinion, hiberantes 2L-QueryCache is not proberly tested by hiberante and therefore still not mature.
I believe that in most realities enabling the 2L-QueryCache hurts more than it benefits.
There's a further problem with 2L-QueryCache which I did adress with
(but again, nobody seems to care about )
Guenther, I haven't forgotten about this thread. I think there is a point in what you're saying but the solution is not clear cut. I'm discussing this with some of my colleagues to see what's the best way forward.
In the mean time, I've made the following change so that if you want, you can subclass InfinispanRegionFactory and override nextTimestamp() so that it has the precision you desired. You can then use your class as region factory. Note that the change I made results on Region implementations using the same nextTimestamp() impl as InfinispanRegionFactory, which was already the case. However now, Region impls call InfinispanRegionFactory.nextTimestamp(), reducing the overriding requirements to simply overriding InfinispanRegionFactory.nextTimestamp(). Previously, you'd have had to override InfinispanRegionFactory and individual region implementatiokns.
Now, a few important notes. Any modifications you make there are at your own risk though, specially when it comes to clustered environments. We do know that query cache configured with asynchronous replication will likely not work as expected if you use a precision like the one you want.
Also, since the fact that Region impls call InfinispanRegionFactory.nextTimestamp() is an implementation detail that might change in the future. So, again, do this at your own risk.
thank you for your efforts.
The solution should work well for us, as most probably we will use a single cluster node.
N.B.: I don't know if you already noticed it, I have found another bug regarding 2L-querycache with Infinispan and opened
Please take a look.
HHH-4836 is now fixed.