This is an automated email from the ASF dual-hosted git repository. tv pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/commons-jcs.git
commit f5c64898fc210e3aa1c515b34fb5361c9027bb25 Author: Thomas Vandahl <[email protected]> AuthorDate: Sun Apr 12 20:32:44 2020 +0200 Get rid of synchronized maps and sets --- .../jcs/auxiliary/disk/jdbc/ShrinkerThread.java | 56 +++++++++------------- .../disk/jdbc/hsql/HSQLDiskCacheFactory.java | 51 ++++++++++---------- .../socket/tcp/LateralTCPDiscoveryListener.java | 38 +++++++-------- .../jcs/JCSConcurrentCacheAccessUnitTest.java | 9 ++-- 4 files changed, 71 insertions(+), 83 deletions(-) diff --git a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/ShrinkerThread.java b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/ShrinkerThread.java index 427cce6..24a9580 100644 --- a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/ShrinkerThread.java +++ b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/ShrinkerThread.java @@ -1,8 +1,7 @@ package org.apache.commons.jcs.auxiliary.disk.jdbc; -import java.util.Collections; -import java.util.HashSet; -import java.util.Set; +import java.util.Iterator; +import java.util.concurrent.CopyOnWriteArraySet; /* * Licensed to the Apache Software Foundation (ASF) under one @@ -39,8 +38,8 @@ public class ShrinkerThread private static final Log log = LogManager.getLog( ShrinkerThread.class ); /** A set of JDBCDiskCache objects to call deleteExpired on. */ - private final Set<JDBCDiskCache<?, ?>> shrinkSet = - Collections.synchronizedSet( new HashSet<>() ); + private final CopyOnWriteArraySet<JDBCDiskCache<?, ?>> shrinkSet = + new CopyOnWriteArraySet<>(); /** Default time period to use. */ private static final long DEFAULT_PAUSE_BETWEEN_REGION_CALLS_MILLIS = 5000; @@ -98,40 +97,29 @@ public class ShrinkerThread log.info( "Running JDBC disk cache shrinker. Number of regions [{0}]", () -> shrinkSet.size() ); - Object[] caches = null; - - synchronized ( shrinkSet ) - { - caches = this.shrinkSet.toArray(); - } - - if ( caches != null ) + for (Iterator<JDBCDiskCache<?, ?>> i = shrinkSet.iterator(); i.hasNext();) { - for ( int i = 0; i < caches.length; i++ ) - { - JDBCDiskCache<?, ?> cache = (JDBCDiskCache<?, ?>) caches[i]; + JDBCDiskCache<?, ?> cache = i.next(); + long start = System.currentTimeMillis(); + int deleted = cache.deleteExpired(); + long end = System.currentTimeMillis(); - long start = System.currentTimeMillis(); - int deleted = cache.deleteExpired(); - long end = System.currentTimeMillis(); + log.info( "Deleted [{0}] expired for region [{1}] for table [{2}] in {3} ms.", + deleted, cache.getCacheName(), cache.getTableName(), end - start ); - log.info( "Deleted [{0}] expired for region [{1}] for table [{2}] in {3} ms.", - deleted, cache.getCacheName(), cache.getTableName(), end - start ); + // don't pause after the last call to delete expired. + if ( i.hasNext() ) + { + log.info( "Pausing for [{0}] ms before shrinking the next region.", + this.getPauseBetweenRegionCallsMillis() ); - // don't pause after the last call to delete expired. - if ( i < caches.length - 1 ) + try + { + Thread.sleep( this.getPauseBetweenRegionCallsMillis() ); + } + catch ( InterruptedException e ) { - log.info( "Pausing for [{0}] ms before shrinking the next region.", - this.getPauseBetweenRegionCallsMillis() ); - - try - { - Thread.sleep( this.getPauseBetweenRegionCallsMillis() ); - } - catch ( InterruptedException e ) - { - log.warn( "Interrupted while waiting to delete expired for the next region." ); - } + log.warn( "Interrupted while waiting to delete expired for the next region." ); } } } diff --git a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/hsql/HSQLDiskCacheFactory.java b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/hsql/HSQLDiskCacheFactory.java index e2e964d..186e46b 100644 --- a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/hsql/HSQLDiskCacheFactory.java +++ b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/jdbc/hsql/HSQLDiskCacheFactory.java @@ -23,9 +23,7 @@ import java.sql.Connection; import java.sql.DriverManager; import java.sql.SQLException; import java.sql.Statement; -import java.util.Collections; -import java.util.HashSet; -import java.util.Set; +import java.util.concurrent.CopyOnWriteArraySet; import org.apache.commons.jcs.auxiliary.AuxiliaryCacheAttributes; import org.apache.commons.jcs.auxiliary.disk.jdbc.JDBCDiskCache; @@ -49,7 +47,7 @@ public class HSQLDiskCacheFactory private static final Log log = LogManager.getLog( HSQLDiskCacheFactory.class ); /** The databases. */ - private Set<String> databases; + private CopyOnWriteArraySet<String> databases; /** * This factory method should create an instance of the hsqlcache. @@ -79,7 +77,7 @@ public class HSQLDiskCacheFactory public void initialize() { super.initialize(); - this.databases = Collections.synchronizedSet( new HashSet<>() ); + this.databases = new CopyOnWriteArraySet<>(); } /** @@ -105,31 +103,34 @@ public class HSQLDiskCacheFactory return; } - // TODO get this from the attributes. - System.setProperty( "hsqldb.cache_scale", "8" ); + synchronized (databases) + { + // TODO get this from the attributes. + System.setProperty( "hsqldb.cache_scale", "8" ); - // "org.hsqldb.jdbcDriver" - String driver = attributes.getDriverClassName(); - // "sa" - String user = attributes.getUserName(); - // "" - String password = attributes.getPassword(); + // "org.hsqldb.jdbcDriver" + String driver = attributes.getDriverClassName(); + // "sa" + String user = attributes.getUserName(); + // "" + String password = attributes.getPassword(); - try - { - Class.forName( driver ).newInstance(); - } - catch (Exception e) - { - throw new SQLException( "Could not initialize driver " + driver, e ); - } + try + { + Class.forName( driver ).newInstance(); + } + catch (Exception e) + { + throw new SQLException( "Could not initialize driver " + driver, e ); + } - Connection cConn = DriverManager.getConnection( database, user, password ); - setupTable( cConn, attributes.getTableName() ); + Connection cConn = DriverManager.getConnection( database, user, password ); + setupTable( cConn, attributes.getTableName() ); - log.info( "Finished setting up database [{0}]", database); + log.info( "Finished setting up database [{0}]", database); - databases.add( database ); + databases.add( database ); + } } /** diff --git a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/lateral/socket/tcp/LateralTCPDiscoveryListener.java b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/lateral/socket/tcp/LateralTCPDiscoveryListener.java index 94d37d8..6816ab2 100644 --- a/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/lateral/socket/tcp/LateralTCPDiscoveryListener.java +++ b/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/lateral/socket/tcp/LateralTCPDiscoveryListener.java @@ -1,11 +1,9 @@ package org.apache.commons.jcs.auxiliary.lateral.socket.tcp; import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.CopyOnWriteArrayList; /* * Licensed to the Apache Software Foundation (ASF) under one @@ -53,15 +51,15 @@ public class LateralTCPDiscoveryListener * Map of no wait facades. these are used to determine which regions are locally configured to * use laterals. */ - private final Map<String, LateralCacheNoWaitFacade<?, ?>> facades = - Collections.synchronizedMap( new HashMap<>() ); + private final ConcurrentMap<String, LateralCacheNoWaitFacade<?, ?>> facades = + new ConcurrentHashMap<>(); /** * List of regions that are configured differently here than on another server. We keep track of * this to limit the amount of info logging. */ - private final Set<String> knownDifferentlyConfiguredRegions = - Collections.synchronizedSet( new HashSet<>() ); + private final CopyOnWriteArrayList<String> knownDifferentlyConfiguredRegions = + new CopyOnWriteArrayList<>(); /** The name of the cache factory */ private String factoryName; @@ -91,7 +89,7 @@ public class LateralTCPDiscoveryListener * @param facade - facade (for region) => multiple lateral clients. * @return true if the facade was not already registered. */ - public synchronized boolean addNoWaitFacade( String cacheName, LateralCacheNoWaitFacade<?, ?> facade ) + public boolean addNoWaitFacade( String cacheName, LateralCacheNoWaitFacade<?, ?> facade ) { boolean isNew = !containsNoWaitFacade( cacheName ); @@ -123,7 +121,9 @@ public class LateralTCPDiscoveryListener public <K, V> boolean containsNoWait( String cacheName, LateralCacheNoWait<K, V> noWait ) { @SuppressWarnings("unchecked") // Need to cast because of common map for all facades - LateralCacheNoWaitFacade<K, V> facade = (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() ); + LateralCacheNoWaitFacade<K, V> facade = + (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() ); + if ( facade == null ) { return false; @@ -141,13 +141,14 @@ public class LateralTCPDiscoveryListener * services. * <p> * @param noWait - * @return true if we found the no wait and added it. False if the no wait was not present or it + * @return true if we found the no wait and added it. False if the no wait was not present or if * we already had it. */ protected <K, V> boolean addNoWait( LateralCacheNoWait<K, V> noWait ) { @SuppressWarnings("unchecked") // Need to cast because of common map for all facades - LateralCacheNoWaitFacade<K, V> facade = (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() ); + LateralCacheNoWaitFacade<K, V> facade = + (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() ); log.debug( "addNoWait > Got facade for {0} = {1}", noWait.getCacheName(), facade ); if ( facade != null ) @@ -158,12 +159,11 @@ public class LateralTCPDiscoveryListener } else { - if ( !knownDifferentlyConfiguredRegions.contains( noWait.getCacheName() ) ) + if ( knownDifferentlyConfiguredRegions.addIfAbsent( noWait.getCacheName() ) ) { log.info( "addNoWait > Different nodes are configured differently " + "or region [{0}] is not yet used on this side.", () -> noWait.getCacheName() ); - knownDifferentlyConfiguredRegions.add( noWait.getCacheName() ); } return false; } @@ -179,7 +179,8 @@ public class LateralTCPDiscoveryListener protected <K, V> boolean removeNoWait( LateralCacheNoWait<K, V> noWait ) { @SuppressWarnings("unchecked") // Need to cast because of common map for all facades - LateralCacheNoWaitFacade<K, V> facade = (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() ); + LateralCacheNoWaitFacade<K, V> facade = + (LateralCacheNoWaitFacade<K, V>)facades.get( noWait.getCacheName() ); log.debug( "removeNoWait > Got facade for {0} = {1}", noWait.getCacheName(), facade); if ( facade != null ) @@ -190,12 +191,11 @@ public class LateralTCPDiscoveryListener } else { - if ( !knownDifferentlyConfiguredRegions.contains( noWait.getCacheName() ) ) + if ( knownDifferentlyConfiguredRegions.addIfAbsent( noWait.getCacheName() ) ) { - log.info( "removeNoWait > Different nodes are configured differently " + log.info( "addNoWait > Different nodes are configured differently " + "or region [{0}] is not yet used on this side.", () -> noWait.getCacheName() ); - knownDifferentlyConfiguredRegions.add( noWait.getCacheName() ); } return false; } diff --git a/commons-jcs-core/src/test/java/org/apache/commons/jcs/JCSConcurrentCacheAccessUnitTest.java b/commons-jcs-core/src/test/java/org/apache/commons/jcs/JCSConcurrentCacheAccessUnitTest.java index 0775fc0..48932cb 100644 --- a/commons-jcs-core/src/test/java/org/apache/commons/jcs/JCSConcurrentCacheAccessUnitTest.java +++ b/commons-jcs-core/src/test/java/org/apache/commons/jcs/JCSConcurrentCacheAccessUnitTest.java @@ -19,16 +19,15 @@ package org.apache.commons.jcs; * under the License. */ -import java.util.ArrayList; -import java.util.Collections; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicInteger; -import junit.framework.TestCase; - import org.apache.commons.jcs.access.GroupCacheAccess; import org.apache.commons.jcs.access.exception.CacheException; +import junit.framework.TestCase; + /** * Test Case for JCS-73, modeled after the Groovy code by Alexander Kleymenov * @@ -67,7 +66,7 @@ public class JCSConcurrentCacheAccessUnitTest extends TestCase JCS.setConfigFilename( "/TestJCS-73.ccf" ); cache = JCS.getGroupCacheInstance( "cache" ); errcount = new AtomicInteger(0); - valueMismatchList = Collections.synchronizedList(new ArrayList<>()); + valueMismatchList = new CopyOnWriteArrayList<>(); } @Override
