Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: 5.6
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4ceb3e
HHH-13613 : Improve CacheImplementor SPI to better support a DomainDa…
gbadner Sep 4, 2019
772d5c2
HHH-13613 : Improve CacheImplementor SPI to better support a DomainDa…
gbadner Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions hibernate-core/src/main/java/org/hibernate/cache/internal/DisabledCaching.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import org.hibernate.cache.cfg.spi.DomainDataRegionConfig;
import org.hibernate.cache.spi.CacheImplementor;
import org.hibernate.cache.spi.DomainDataRegion;
import org.hibernate.cache.spi.QueryResultsCache;
import org.hibernate.cache.spi.Region;
import org.hibernate.cache.spi.RegionFactory;
Expand Down Expand Up @@ -183,7 +184,26 @@ public String[] getSecondLevelCacheRegionNames() {
}

@Override
@SuppressWarnings("unchecked")
public Set<String> getCacheRegionNames() {
return Collections.EMPTY_SET;
}

@Override
@SuppressWarnings("unchecked")
public Set<String> getDomainDataRegionNames() {
return Collections.EMPTY_SET;
}

@Override
@SuppressWarnings("unchecked")
public Set<String> getQueryCacheRegionNames() {
return Collections.EMPTY_SET;
}

@Override
@SuppressWarnings("unchecked")
public DomainDataRegion getDomainDataRegion(String regionName) {
return null;
}

Expand Down
109 changes: 71 additions & 38 deletions hibernate-core/src/main/java/org/hibernate/cache/internal/EnabledCaching.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,9 @@ public class EnabledCaching implements CacheImplementor, DomainDataRegionBuildin
private final SessionFactoryImplementor sessionFactory;
private final RegionFactory regionFactory;

private final Map<String,Region> regionsByName = new ConcurrentHashMap<>();
private final Set<String> domainDataAndQueryResultsRegionNames = new HashSet<>();

// A map by name for QueryResultsRegion instances that have the same name as a Region
// in #regionsByName.
private final Map<String, QueryResultsRegion> queryResultsRegionsByDuplicateName = new ConcurrentHashMap<>();
private final Map<String, DomainDataRegion> domainDataRegionMap = new ConcurrentHashMap<>();

private final Map<NavigableRole,EntityDataAccess> entityAccessMap = new ConcurrentHashMap<>();
private final Map<NavigableRole,NaturalIdDataAccess> naturalIdAccessMap = new ConcurrentHashMap<>();
Expand Down Expand Up @@ -95,11 +93,12 @@ public EnabledCaching(SessionFactoryImplementor sessionFactory) {
RegionFactory.DEFAULT_QUERY_RESULTS_REGION_UNQUALIFIED_NAME,
sessionFactory
);
regionsByName.put( queryResultsRegion.getName(), queryResultsRegion );
domainDataAndQueryResultsRegionNames.add( queryResultsRegion.getName() );
Copy link
Contributor Author

@gbadner gbadner Sep 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
		)
);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

defaultQueryResultsCache = new QueryResultsCacheImpl(
queryResultsRegion,
timestampsCache
);
// defaultQueryResultsCache is not included in namedQueryResultsCacheMap.
}
else {
timestampsCache = new TimestampsCacheDisabledImpl();
Expand All @@ -111,7 +110,8 @@ public EnabledCaching(SessionFactoryImplementor sessionFactory) {
public void prime(Set<DomainDataRegionConfig> cacheRegionConfigs) {
for ( DomainDataRegionConfig regionConfig : cacheRegionConfigs ) {
final DomainDataRegion region = getRegionFactory().buildDomainDataRegion( regionConfig, this );
regionsByName.put( region.getName(), region );
domainDataAndQueryResultsRegionNames.add( region.getName() );
domainDataRegionMap.put( region.getName(), region );

if ( ! Objects.equals( region.getName(), regionConfig.getRegionName() ) ) {
throw new HibernateException(
Expand Down Expand Up @@ -209,12 +209,26 @@ public TimestampsCache getTimestampsCache() {

@Override
public Region getRegion(String regionName) {
// The Region in regionsByName has precedence over the
// QueryResultsRegion in #queryResultsRegionsByDuplicateName
return regionsByName.get( regionName );
}

// The Region in domainDataRegionMap has precedence over the
// QueryResultsCache in #namedQueryResultsCacheMap

final Region region = domainDataRegionMap.get( regionName );
if ( region != null ) {
return region;
}
else if ( !getSessionFactory().getSessionFactoryOptions().isQueryCacheEnabled() ) {
return null;
}
else if ( defaultQueryResultsCache.getRegion().getName().equals( regionName ) ) {
return defaultQueryResultsCache.getRegion();
}
else {
final QueryResultsCache queryResultsCache = namedQueryResultsCacheMap.get( regionName );
return queryResultsCache != null
? queryResultsCache.getRegion()
: null;
}
}

// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// Entity data
Expand Down Expand Up @@ -428,7 +442,7 @@ public void evictDefaultQueryRegion() {

@Override
public void evictQueryRegion(String regionName) {
final QueryResultsCache cache = getQueryResultsCache( regionName );
final QueryResultsCache cache = namedQueryResultsCacheMap.get( regionName );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if ( cache == null ) {
return;
}
Expand All @@ -454,6 +468,8 @@ public void evictQueryRegions() {
LOG.debug( "Evicting cache of all query regions." );
}

// defaultQueryResultsCache is not in namedQueryResultsCacheMap so
// evict it separately.
evictQueryResultRegion( defaultQueryResultsCache );

for ( QueryResultsCache cache : namedQueryResultsCacheMap.values() ) {
Expand All @@ -472,7 +488,6 @@ public QueryResultsCache getQueryResultsCache(String regionName) throws Hibernat
return null;
}


if ( regionName == null || regionName.equals( getDefaultQueryResultsCache().getRegion().getName() ) ) {
return getDefaultQueryResultsCache();
}
Expand All @@ -491,32 +506,26 @@ public QueryResultsCache getQueryResultsCacheStrictly(String regionName) {
return null;
}

// namedQueryResultsCacheMap does not include defaultQueryResultsCache
return namedQueryResultsCacheMap.get( regionName );
}

protected QueryResultsCache makeQueryResultsRegionAccess(String regionName) {
final Region region = regionsByName.computeIfAbsent(
final QueryResultsCache regionAccess = namedQueryResultsCacheMap.computeIfAbsent(
regionName,
this::makeQueryResultsRegion
this::makeQueryResultsCache
);
final QueryResultsRegion queryResultsRegion;
if ( QueryResultsRegion.class.isInstance( region ) ) {
queryResultsRegion = (QueryResultsRegion) region;
}
else {
// There was already a different type of Region with the same name.
queryResultsRegion = queryResultsRegionsByDuplicateName.computeIfAbsent(
regionName,
this::makeQueryResultsRegion
);
}
final QueryResultsCacheImpl regionAccess = new QueryResultsCacheImpl(
domainDataAndQueryResultsRegionNames.add( regionName );
legacySecondLevelCacheNames.add( regionName );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ) 
);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return regionAccess;
}

private QueryResultsCache makeQueryResultsCache(String regionName) {
final QueryResultsRegion queryResultsRegion = makeQueryResultsRegion( regionName );
return new QueryResultsCacheImpl(
queryResultsRegion,
timestampsCache
);
namedQueryResultsCacheMap.put( regionName, regionAccess );
legacySecondLevelCacheNames.add( regionName );
return regionAccess;
}

protected QueryResultsRegion makeQueryResultsRegion(String regionName) {
Expand All @@ -525,15 +534,34 @@ protected QueryResultsRegion makeQueryResultsRegion(String regionName) {

@Override
public Set<String> getCacheRegionNames() {
return regionsByName.keySet();
return domainDataAndQueryResultsRegionNames;
}

@Override
public Set<String> getDomainDataRegionNames() {
return domainDataRegionMap.keySet();
}

@Override
public Set<String> getQueryCacheRegionNames() {
return namedQueryResultsCacheMap.keySet();
}

@Override
public DomainDataRegion getDomainDataRegion(String regionName) {
return domainDataRegionMap.get( regionName );
}

@Override
public void evictRegion(String regionName) {
getRegion( regionName ).clear();
final QueryResultsRegion queryResultsRegionWithDuplicateName = queryResultsRegionsByDuplicateName.get( regionName );
if ( queryResultsRegionWithDuplicateName != null ) {
queryResultsRegionWithDuplicateName.clear();
final DomainDataRegion domainDataRegion = domainDataRegionMap.get( regionName );
if ( domainDataRegion != null ) {
domainDataRegion.clear();
}

final QueryResultsCache queryResultsCache = namedQueryResultsCacheMap.get( regionName );
if ( queryResultsCache != null ) {
queryResultsCache.getRegion().clear();
}
}

Expand All @@ -553,11 +581,16 @@ public <T> T unwrap(Class<T> cls) {

@Override
public void close() {
for ( Region region : regionsByName.values() ) {
for ( Region region : domainDataRegionMap.values() ) {
region.destroy();
}
for ( Region region : queryResultsRegionsByDuplicateName.values() ) {
region.destroy();

if ( defaultQueryResultsCache != null ) {
defaultQueryResultsCache.getRegion().destroy();
}

for ( QueryResultsCache queryResultsCache : namedQueryResultsCacheMap.values() ) {
queryResultsCache.getRegion().destroy();
}
}

Expand Down
96 changes: 92 additions & 4 deletions hibernate-core/src/main/java/org/hibernate/cache/spi/CacheImplementor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
package org.hibernate.cache.spi;

import java.io.Serializable;
import java.util.HashSet;
import java.util.Locale;
import java.util.Set;

Expand All @@ -28,6 +29,7 @@
*
* @author Strong Liu
* @author Steve Ebersole
* @author Gail Badner
*/
@SuppressWarnings("unused")
public interface CacheImplementor extends Service, Cache, org.hibernate.engine.spi.CacheImplementor, Serializable {
Expand Down Expand Up @@ -55,21 +57,100 @@ public interface CacheImplementor extends Service, Cache, org.hibernate.engine.s
* Get a cache Region by name. If there is both a {@link DomainDataRegion}
* and a {@link QueryResultsRegion} with the specified name, then the
* {@link DomainDataRegion} will be returned.
*
* <p/>
* If Hibernate is configured for query result caching, and the {@code regionName}
* is the name of the default {@link QueryResultsRegion}, then the default {@link QueryResultsRegion}
* will be returned.
* <p/>
* @apiNote It is only valid to call this method after {@link #prime} has
* been performed
*
* @since 5.3
*
* @return the {@link Region} or {@code null} if there is none with the specified name.
*
* @deprecated to access a {@link DomainDataRegion}, use {@link #getDomainDataRegion(String)};
* to access the default {@link QueryResultsRegion}, use {@link #getDefaultQueryResultsCache}.getRegion();
* to access a named (non-default) {@link QueryResultsRegion}, use
* {@link #getQueryResultsCacheStrictly(String)}.getRegion().
*/
@Deprecated
Region getRegion(String regionName);

/**
* The unqualified name of all regions. Intended for use with {@link #getRegion}
* <p/>
* If Hibernate is configured for query result caching, then the name of the
* default {@link QueryResultsRegion} will be included in the returned Set.
*
* @since 5.3
* @deprecated Use {@link #getDomainDataRegionNames} to get all {@link DomainDataRegion} names;
* use {@link #getQueryCacheRegionNames()} to get all named (non-default) {@link DomainDataRegion}
* names; use {@link #getDefaultQueryResultsCache()}.getRegion().getName() to get the name of
* the default {@link QueryResultsRegion}.
*/
@Deprecated
Set<String> getCacheRegionNames();

/**
* The unqualified names of all {@link DomainDataRegion} regions. Intended for use with
* {@link #getDomainDataRegion(String)}.
*
* @apiNote It is only valid to call this method after {@link #prime} has
* been performed
*
* @since 5.3
*/
default Set<String> getDomainDataRegionNames() {
final Set<String> domainDataRegionNames = new HashSet<>();
for ( String regionName : getCacheRegionNames() ) {
final Region region = getRegion( regionName );
if ( DomainDataRegion.class.isInstance( region ) ) {
domainDataRegionNames.add( regionName );
}
}
return domainDataRegionNames;
}

/**
* The unqualified names of all named (non-default) {@link QueryResultsRegion} regions. Intended for use with
* {@link #getQueryResultsCacheStrictly(String)}.
* <p/>
* The name of the default {@link QueryResultsRegion} will not be included in the returned Set.
* {@link #getDefaultQueryResultsCache()} should be used to get access to the default
* {@link QueryResultsCache}.
*
* @since 5.3
*/
default Set<String> getQueryCacheRegionNames() {
final Set<String> queryCacheRegionNames = new HashSet<>();
for ( String regionName : getCacheRegionNames() ) {
final QueryResultsCache queryResultsCache = getQueryResultsCacheStrictly( regionName );
if ( queryResultsCache != null ) {
queryCacheRegionNames.add( queryResultsCache.getRegion().getName() );
}
}
return queryCacheRegionNames;
}

/**
* Get a {@link DomainDataRegion} by name.
*
* @apiNote It is only valid to call this method after {@link #prime} has
* been performed
*
* @return the {@link DomainDataRegion} with the specified name, or {@code null}
* if there is no {@link DomainDataRegion} with that name.
*
* @since 5.3
*/
default DomainDataRegion getDomainDataRegion(String regionName) {
final Region region = getRegion( regionName );
return DomainDataRegion.class.isInstance( region )
? (DomainDataRegion) region
: null;
}

/**
* Find the cache data access strategy for Hibernate's timestamps cache.
* Will return {@code null} if Hibernate is not configured for query result caching
Expand Down Expand Up @@ -97,9 +178,13 @@ public interface CacheImplementor extends Service, Cache, org.hibernate.engine.s
/**
* Get the named QueryResultRegionAccess but not creating one if it
* does not already exist. This is intended for use by statistics.
*
* <p/>
* Will return {@code null} if Hibernate is not configured for query result
* caching or if no such region (yet) exists
* caching, if no such region (yet) exists, or if {@code regionName} is the
* name of the default {@link QueryResultsRegion}.
* <p/>
* {@link #getDefaultQueryResultsCache()} should be used to get access to the default
* {@link QueryResultsCache}.
*
* @since 5.3
*/
Expand Down Expand Up @@ -129,7 +214,10 @@ default void evictQueries() throws HibernateException {
*
* @return All cache region names
*
* @deprecated (since 5.3) Use {@link CacheImplementor#getCacheRegionNames()} instead
* @deprecated (since 5.3) Use {@link #getDomainDataRegionNames} to get all {@link DomainDataRegion} names;
* use {@link #getQueryCacheRegionNames()} to get all named (non-default) {@link DomainDataRegion}
* names; use {@link #getDefaultQueryResultsCache()}.getRegion().getName() to get the name of
* the default {@link QueryResultsRegion}.
*/
@Deprecated
String[] getSecondLevelCacheRegionNames();
Expand Down
4 changes: 2 additions & 2 deletions hibernate-core/src/main/java/org/hibernate/stat/internal/StatisticsImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ public CacheRegionStatisticsImpl getDomainDataRegionStatistics(String regionName
return l2CacheStatsMap.getOrCompute(
regionName,
s -> {
final Region region = cache.getRegion( s );
final Region region = cache.getDomainDataRegion( s );

if ( region == null ) {
throw new IllegalArgumentException( "Unknown cache region : " + s );
Expand Down Expand Up @@ -609,7 +609,7 @@ public CacheRegionStatisticsImpl getCacheRegionStatistics(String regionName) {
return l2CacheStatsMap.getOrCompute(
regionName,
s -> {
Region region = cache.getRegion( s );
Region region = cache.getDomainDataRegion( s );

if ( region == null ) {

Expand Down
Loading