[5.6] HHH-13613 : Improve CacheImplementor SPI to better support a DomainDa…

[gbadner]

…taRegion and QueryResultsRegion with the same name

https://hibernate.atlassian.net/browse/HHH-13613

See the previous PR for earlier conversation: #3006

[gbadner]

The default QueryResultsRegion is not added to #legacySecondLevelCacheNames. Should it be?

If so, should it be qualified?

legacySecondLevelCacheNames.add(
		StringHelper.qualifyConditionally(
					getSessionFactory().getSessionFactoryOptions().getCacheRegionPrefix(),
					queryResultsRegion.getName()
		)
);

[sebersole]

Practically speaking the default region does have a name, but conceptually that is not the case. No it should not be added to legacySecondLevelCacheNames

[gbadner]

#getQueryResultsCache will create a new QueryResultsCache if one does not exist with the provided regionName.

I didn't think that it made sense to create a new QueryResultsCache when evicting, so I changed the line to lookup only an existing.

Should this be reverted?

[sebersole]

I think not creating the region on evict is better. So I'd leave your change

[gbadner]

The new named QueryResultsRegion is not qualified when added to #legacySecondLevelCacheNames.

Should it be qualified as follows?

legacySecondLevelCacheNames.add( StringHelper.qualifyConditionally( 
        getSessionFactory().getSessionFactoryOptions().getCacheRegionPrefix(), 
        regionName ) 
);

[sebersole]

Actually, I believe that the legacy behavior was to not expect the user to prefix the names for query regions which is different than its expectations for domain data regions. Not completely sure, so we should maybe verify that.

[gbadner]

StatisticsImpl#getCacheRegionStatistics and getQueryRegionStats call CacheImplementor#getQueryResultsCache.

Should a QueryResultsRegion really be created if one does not exist yet?

I suppose it makes sense to do this, if the application will ultimately create the region.

WDYT?

[sebersole]

StatisticsImpl#getCacheRegionStatistics and getQueryRegionStats call CacheImplementor#getQueryResultsCache.

Should a QueryResultsRegion really be created if one does not exist yet?

The stats reference itself should be created. So from there it depends whether you want to have a mutable CacheRegionStatisticsImpl to be able to inject the Region later if need be or to just create the Region up front to pass along to the CacheRegionStatisticsImpl. IMO creating the Region when we create the CacheRegionStatisticsImpl makes the most sense.

[sebersole]

Actually, #getQueryRegionStats is only triggered when the stats are being updated because of the execution of a query - it is only ever called from StatisticsImpl#queryCacheHit, StatisticsImpl#queryCacheMiss, etc. Here we fully expect the region to exist.

WRT StatisticsImpl#getCacheRegionStatistics, as outlined in its comments it is a deprecated legacy method and its implementation it intended to be consistent with the legacy expectation. Its replacement is StatisticsImpl#getQueryRegionStatistics which correctly uses CacheImplementor#getQueryResultsCacheStrictly instead

[hibernate-github-bot]

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ The pull request title should not end with an ellipsis (make sure the title is complete)
❌ The pull request description must contain the license agreement text.
    ↳ The description of this pull request must contain the following license agreement text:

----------------------
By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license](https://www.apache.org/licenses/LICENSE-2.0.txt)
and can be relicensed under the terms of the [LGPL v2.1 license](https://www.gnu.org/licenses/old-licenses/lgpl-2.1.txt) in the future at the maintainers' discretion.
For more information on licensing, please check [here](https://github.com/hibernate/hibernate-orm/blob/main/CONTRIBUTING.md#legal).

----------------------

› This message was automatically generated.