HHH-7898 Regression on org.hibernate.cache.infinispan.query.QueryResu…

[rvansa]

…ltsRegionImpl.put(Object, Object)

• Moved query cache update to second phase of transaction commit
• Query caches are now recommended to be non-transactional (transactional ones will be slower)

JIRA: https://hibernate.atlassian.net/browse/HHH-7898

[rvansa]

One thing worth mentioning: with this fix, if the same transaction executes the same query, the value is not in transactional context nor committed in the cache and therefore, it will result in cache miss.
This is not revealed by current tests since the cache was made non-transactional, and the region detects current transaction based on the transaction manager retrieved from the cache, not from the transaction manager used to executed the whole operation. Therefore, the tests execute non-transactional code.

[rvansa]

I've added a logic that does not break one of the unit tests, but it's based on some other changes that await as PR. Though, I am not sure if I should implement the same logic for non-JTA transactions - currently it simply writes the entry before the transaction commits. However, can I execute another JDBC transaction from afterCompletion? TransactionCoordinator does not have any API for suspend.

[sebersole]

TransactionCoordinator tc = ...;
tc.createIsolationDelegate().delegateWork( ... )

Whatever work is done here is isolated from the current transaction. And it works for both JTA and JDBC. Starting to see the benefit of encapsulating all this? ;)

[rvansa]

@sebersole Ah, ok :) I'll make this TX impl agnostic :)

[rvansa]

@sebersole Using the isolation delegate simplifies the code a lot, though, it would be better if I didn't have to obtain the connection when I don't need it. Should I add a
<T> T delegateCallable(Callable<T> callable, boolean transacted) throws HibernateException
to IsolationDelegate?

[rvansa]

Reworked... I had to reshuffle the query cache tests a bit to correctly use sessions, but it was worth it.

[sebersole]

@rvansa We already have the Connection. There is no overhead in it being there. -1 for adding a new method just to not accept it.

[sebersole]

@rvansa On HHH-7898 you mentioned that this work also needed to account for HHH-9988. But I do not see a PR for that. Did you address that one here? Somewhere else? Not at all?

[rvansa]

@sebersole It seemed to me that JtaIsolationDelegate.doTheWork obtains separate connection, if it does not, my mistake.

Wrt HHH-9988: When using TransactionCoordinator, HHH-9988 is not necessary in fact, I'll correct it. Sorry about making things messy with several related JIRAs awaiting PRs. Btw., is there any recommended practice to tag the PR when it depends on another not-integrated changes?

[sebersole]

@rvansa actually you are right I think in regards to the Connection. I guess I am ok with adding a new method there. Let me know if you'd prefer me to work on that.

In regards to HHH-9988, awesome. So should HHH-9988 be closed?

In regards to "dependent PRs".. The best you can refer to them and make sure everything is cross linked. For now, maybe ping me on IRC and walk me through which need applied?

[dreab8]

Thanks @rvansa ,

PR fixed conflicts, rebased and merged