Author: manik.surtani(a)jboss.com Date: 2008-07-30 11:45:12 -0400 (Wed, 30 Jul 2008) New Revision: 6443 Modified: core/trunk/src/main/java/org/jboss/cache/LegacyRegionManagerImpl.java core/trunk/src/main/java/org/jboss/cache/RegionManagerImpl.java Log: Use StripedLock instead of a Set to keep track of concurrent activations and deactivations!!! cached log trace level Modified: core/trunk/src/main/java/org/jboss/cache/LegacyRegionManagerImpl.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/LegacyRegionManagerImpl.java 2008-07-30 15:43:05 UTC (rev 6442) +++ core/trunk/src/main/java/org/jboss/cache/LegacyRegionManagerImpl.java 2008-07-30 15:45:12 UTC (rev 6443) @@ -32,14 +32,6 @@ @Override protected void inactivateRegion(Fqn fqn) throws CacheException { - if (isActivatingDeactivating(fqn)) - { -// throw new CacheException("Region " + fqn + " is already being activated/deactivated"); - log.warn("Region " + fqn + " is already being activated/deactivated"); - return; - } - - NodeSPI parent = null; NodeSPI subtreeRoot = null; boolean parentLocked = false; @@ -48,7 +40,7 @@ try { // Record that this fqn is in status change, so can't provide state - activationChangeNodes.add(fqn); + lock(fqn); if (!isInactive(fqn)) { @@ -142,7 +134,7 @@ } } - activationChangeNodes.remove(fqn); + unlock(fqn); } } Modified: core/trunk/src/main/java/org/jboss/cache/RegionManagerImpl.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/RegionManagerImpl.java 2008-07-30 15:43:05 UTC (rev 6442) +++ core/trunk/src/main/java/org/jboss/cache/RegionManagerImpl.java 2008-07-30 15:45:12 UTC (rev 6443) @@ -19,11 +19,12 @@ import org.jboss.cache.invocation.InvocationContext; import org.jboss.cache.lock.LockManager; import static org.jboss.cache.lock.LockType.WRITE; +import org.jboss.cache.util.concurrent.locks.LockContainer; +import org.jboss.cache.util.concurrent.locks.ReentrantLockContainer; import org.jgroups.Address; import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Set; @@ -45,19 +46,39 @@ private RegionRegistry regionsRegistry; private boolean defaultInactive; - protected final Log log = LogFactory.getLog(RegionManagerImpl.class); + protected static final Log log = LogFactory.getLog(RegionManagerImpl.class); + protected static final boolean trace = log.isTraceEnabled(); CacheSPI<?, ?> cache; private boolean usingEvictions; private EvictionConfig evictionConfig; private final EvictionTimerTask evictionTimerTask = new EvictionTimerTask(); - protected final Set<Fqn> activationChangeNodes = Collections.synchronizedSet(new HashSet<Fqn>()); + private final LockContainer<Fqn> regionLocks = new ReentrantLockContainer<Fqn>(4); protected Configuration configuration; protected RPCManager rpcManager; protected LockManager lockManager; protected BuddyFqnTransformer buddyFqnTransformer; private boolean isUsingBR; + // -------- region lock helpers + + protected final boolean isRegionLocked(Fqn fqn) + { + return regionLocks.isLocked(fqn); + } + + protected final void lock(Fqn fqn) + { + regionLocks.getLock(fqn).lock(); + if (trace) log.trace("LOCKED " + fqn, new Throwable()); + } + + protected final void unlock(Fqn fqn) + { + if (trace) log.trace("UNLOCKED " + fqn, new Throwable()); + regionLocks.getLock(fqn).unlock(); + } + @Inject public void injectDependencies(CacheSPI cache, Configuration configuration, RPCManager rpcManager, LockManager lockManager, BuddyFqnTransformer transformer, RegionRegistry regionsRegistry) @@ -109,7 +130,7 @@ protected void destroy() { regionsRegistry.clear(); - activationChangeNodes.clear(); + regionLocks.reset(); } public boolean isUsingEvictions() @@ -169,7 +190,7 @@ fqn = buddyFqnTransformer.getActualFqn(fqn); } - if (log.isTraceEnabled()) log.trace("Contents of RegionsRegistry: " + regionsRegistry); + if (trace) log.trace("Contents of RegionsRegistry: " + regionsRegistry); Fqn fqnToUse = fqn; if (DEFAULT_REGION.equals(fqnToUse)) fqnToUse = Fqn.ROOT; // first see if a region for this specific Fqn exists @@ -210,7 +231,7 @@ if (regionsRegistry.containsKey(nextFqn)) { Region r = regionsRegistry.get(nextFqn); - if (log.isTraceEnabled()) log.trace("Trying next region " + nextFqn + " and got " + r); + if (trace) log.trace("Trying next region " + nextFqn + " and got " + r); // this is a very poor way of telling whether a region is a marshalling one or an eviction one. :-( // mandates that class loaders be registered for marshalling regions. @@ -271,7 +292,7 @@ { try { - if (log.isTraceEnabled()) log.trace("Activating region " + fqn); + if (trace) log.trace("Activating region " + fqn); Region r = getRegion(fqn, false); if (r != null) { @@ -339,11 +360,6 @@ // Check whether the node already exists and has data Node subtreeRoot = cache.getNode(fqn); // NOTE this used to be a peek! - if (isActivatingDeactivating(fqn)) - { - throw new CacheException("Region " + fqn + " is already being activated/deactivated"); - } - if (log.isDebugEnabled()) { log.debug("activating " + fqn); @@ -354,7 +370,7 @@ // Add this fqn to the set of those we are activating // so calls to _getState for the fqn can return quickly - activationChangeNodes.add(fqn); + lock(fqn); BuddyManager buddyManager = cache.getBuddyManager(); // Request partial state from the cluster and integrate it @@ -366,8 +382,9 @@ { // We'll update this node with the state we receive // need to obtain all necessary locks. + Node root = cache.getRoot(); cache.getInvocationContext().getOptionOverrides().setCacheModeLocal(true); - subtreeRoot = cache.getRoot().addChild(fqn); + subtreeRoot = root.addChild(fqn); cache.getInvocationContext().getOptionOverrides().setCacheModeLocal(false); } @@ -440,7 +457,7 @@ } finally { - activationChangeNodes.remove(fqn); + unlock(fqn); } } @@ -466,13 +483,6 @@ */ protected void inactivateRegion(Fqn fqn) throws CacheException { - if (isActivatingDeactivating(fqn)) - { - log.warn("Region " + fqn + " is already being activated/deactivated. Not doing anything."); - return; - } -// throw new CacheException("Region " + fqn + " is already being activated/deactivated"); - NodeSPI parent = null; NodeSPI subtreeRoot = null; boolean parentLocked = false; @@ -483,7 +493,7 @@ try { // Record that this fqn is in status change, so can't provide state - activationChangeNodes.add(fqn); + lock(fqn); if (!isInactive(fqn)) { @@ -579,24 +589,10 @@ // If necessary, release locks if (ctx != null) lockManager.unlock(ctx); - activationChangeNodes.remove(fqn); + unlock(fqn); } } - /** - * <p/> - * This is legacy code and should not be called directly. This is a private method for now and will be refactored out. - * You should be using {@link #activate(Fqn)} and {@link #deactivate(Fqn)} - * <p/> - * - * @param fqn fqn of the region - * @return true if the region defined by the fqn is in the process of activating/deactivating - */ - protected boolean isActivatingDeactivating(Fqn fqn) - { - return activationChangeNodes.contains(fqn); - } - public boolean hasRegion(Fqn fqn, Region.Type type) { Region r = regionsRegistry.get(fqn); @@ -728,7 +724,7 @@ for (EvictionRegionConfig erc : ercs) { Fqn fqn = erc.getRegionFqn(); - if (log.isTraceEnabled()) log.trace("Creating eviction region " + fqn); + if (trace) log.trace("Creating eviction region " + fqn); if (fqn.equals(DEFAULT_REGION)) { @@ -736,7 +732,7 @@ { throw new ConfigurationException("A default region for evictions has already been set for this cache"); } - if (log.isTraceEnabled()) log.trace("Applying settings for " + DEFAULT_REGION + " to Fqn.ROOT"); + if (trace) log.trace("Applying settings for " + DEFAULT_REGION + " to Fqn.ROOT"); fqn = Fqn.ROOT; setDefault = true; }