Author: manik.surtani(a)jboss.com Date: 2008-07-31 10:50:35 -0400 (Thu, 31 Jul 2008) New Revision: 6462 Modified: core/trunk/src/main/java/org/jboss/cache/PessimisticUnversionedNode.java core/trunk/src/main/java/org/jboss/cache/UnversionedNode.java core/trunk/src/main/java/org/jboss/cache/interceptors/BaseRpcInterceptor.java core/trunk/src/main/java/org/jboss/cache/interceptors/CacheStoreInterceptor.java core/trunk/src/main/java/org/jboss/cache/interceptors/PessimisticLockInterceptor.java core/trunk/src/main/java/org/jboss/cache/interceptors/TxInterceptor.java core/trunk/src/main/java/org/jboss/cache/invocation/AbstractInvocationContext.java core/trunk/src/main/java/org/jboss/cache/invocation/InvocationContext.java core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeFactory.java core/trunk/src/main/java/org/jboss/cache/transaction/MVCCTransactionContext.java core/trunk/src/main/java/org/jboss/cache/transaction/PessimisticTransactionContext.java core/trunk/src/main/java/org/jboss/cache/transaction/TransactionContext.java Log: Refactored the way modification lists are managed in the transaction context Modified: core/trunk/src/main/java/org/jboss/cache/PessimisticUnversionedNode.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/PessimisticUnversionedNode.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/PessimisticUnversionedNode.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -169,7 +169,7 @@ if (gtx != null) { CreateNodeCommand createNodeCommand = commandsFactory.buildCreateNodeCommand(childFqn); - ctx.getTransactionContext().addModification(createNodeCommand); + ctx.getTransactionContext().addLocalModification(createNodeCommand); } if (notify) cache.getNotifier().notifyNodeCreated(childFqn, false, ctx); } Modified: core/trunk/src/main/java/org/jboss/cache/UnversionedNode.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/UnversionedNode.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/UnversionedNode.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -322,7 +322,7 @@ if (ctx.getTransactionContext() != null) { CreateNodeCommand createNodeCommand = commandsFactory.buildCreateNodeCommand(childFqn); - ctx.getTransactionContext().addModification(createNodeCommand); + ctx.getTransactionContext().addLocalModification(createNodeCommand); } // notify if we actually created a new child Modified: core/trunk/src/main/java/org/jboss/cache/interceptors/BaseRpcInterceptor.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/interceptors/BaseRpcInterceptor.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/interceptors/BaseRpcInterceptor.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -162,7 +162,7 @@ protected boolean skipReplicationOfTransactionMethod(InvocationContext ctx) { GlobalTransaction gtx = ctx.getGlobalTransaction(); - return ctx.getTransaction() == null || gtx == null || gtx.isRemote() || ctx.getOptionOverrides().isCacheModeLocal() || !ctx.isTxHasMods(); + return ctx.getTransaction() == null || gtx == null || gtx.isRemote() || ctx.getOptionOverrides().isCacheModeLocal() || !ctx.getTransactionContext().hasModifications(); } /** Modified: core/trunk/src/main/java/org/jboss/cache/interceptors/CacheStoreInterceptor.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/interceptors/CacheStoreInterceptor.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/interceptors/CacheStoreInterceptor.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -100,7 +100,7 @@ { if (inTransaction()) { - if (ctx.isTxHasMods()) + if (ctx.getTransactionContext().hasAnyModifications()) { // this is a commit call. GlobalTransaction gtx = command.getGlobalTransaction(); @@ -154,7 +154,7 @@ if (inTransaction()) { if (trace) log.trace("transactional so don't put stuff in the cloader yet."); - if (ctx.isTxHasMods()) + if (ctx.getTransactionContext().hasAnyModifications()) { GlobalTransaction gtx = command.getGlobalTransaction(); // this is a rollback method Modified: core/trunk/src/main/java/org/jboss/cache/interceptors/PessimisticLockInterceptor.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/interceptors/PessimisticLockInterceptor.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/interceptors/PessimisticLockInterceptor.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -39,6 +39,7 @@ import org.jboss.cache.lock.LockUtil; import org.jboss.cache.lock.PessimisticNodeBasedLockManager; import org.jboss.cache.transaction.GlobalTransaction; +import org.jboss.cache.transaction.PessimisticTransactionContext; import org.jboss.cache.transaction.TransactionContext; import java.util.ArrayList; @@ -159,7 +160,7 @@ } // 1. Revert the modifications by running the undo-op list in reverse. This *cannot* throw any exceptions ! - undoOperations(transactionContext); + undoOperations((PessimisticTransactionContext) transactionContext); } if (trace) { @@ -170,9 +171,9 @@ return retVal; } - private void undoOperations(TransactionContext transactionContext) + private void undoOperations(PessimisticTransactionContext transactionContext) { - List<WriteCommand> modificationList = transactionContext.getModifications(); + List<WriteCommand> modificationList = transactionContext.getAllModifications(); if (modificationList.isEmpty()) { Modified: core/trunk/src/main/java/org/jboss/cache/interceptors/TxInterceptor.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/interceptors/TxInterceptor.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/interceptors/TxInterceptor.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -19,7 +19,6 @@ import org.jboss.cache.commands.tx.PrepareCommand; import org.jboss.cache.commands.tx.RollbackCommand; import org.jboss.cache.commands.write.ClearDataCommand; -import org.jboss.cache.commands.write.CreateNodeCommand; import org.jboss.cache.commands.write.InvalidateCommand; import org.jboss.cache.commands.write.PutDataMapCommand; import org.jboss.cache.commands.write.PutKeyValueCommand; @@ -45,7 +44,6 @@ import javax.transaction.Transaction; import java.util.Collections; import java.util.HashMap; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; @@ -560,8 +558,6 @@ */ protected void runCommitPhase(InvocationContext ctx, GlobalTransaction gtx, List modifications, boolean onePhaseCommit) { - // set the hasMods flag in the invocation ctx. This should not be replicated, just used locally by the interceptors. - ctx.setTxHasMods(modifications != null && modifications.size() > 0); try { VisitableCommand commitCommand = onePhaseCommit ? buildPrepareCommand(gtx, modifications, true) : commandsFactory.buildCommitCommand(gtx); @@ -603,12 +599,10 @@ /** * creates a rollback() */ - protected void runRollbackPhase(InvocationContext ctx, GlobalTransaction gtx, Transaction tx, List<WriteCommand> modifications) + protected void runRollbackPhase(InvocationContext ctx, GlobalTransaction gtx, Transaction tx) { try { - ctx.setTxHasMods(modifications != null && modifications.size() > 0); - // JBCACHE-457 VisitableCommand rollbackCommand = commandsFactory.buildRollbackCommand(gtx); if (trace) log.trace(" running rollback for " + gtx); @@ -629,25 +623,6 @@ } } - /** - * An optimisation of commands to be broadcast in a prepare call, which involves removing of unnecessary commands - * as well as the "aggregation" of commands that can be aggregated. - * - * @param mods list of modifications - * @return compacted list of modifications - */ - private List<WriteCommand> compact(List<WriteCommand> mods) - { - // TODO: 3.0.0: Make this more sophisticated, so it aggregates multiple puts on the same node, puts followed by a remove, etc. - // for now this just removes the redundant CreateNodeCommands from the list. - List<WriteCommand> newList = new LinkedList<WriteCommand>(); - for (WriteCommand cmd : mods) - { - if (!(cmd instanceof CreateNodeCommand)) newList.add(cmd); - } - return newList; - } - private boolean isOnePhaseCommit() { if (!configuration.getCacheMode().isSynchronous() && !optimistic) @@ -680,8 +655,6 @@ { VisitableCommand originalCommand = ctx.getCommand(); ctx.setCommand(prepareCommand); - // set the hasMods flag in the invocation ctx. This should not be replicated, just used locally by the interceptors. - ctx.setTxHasMods(modifications != null && modifications.size() > 0); try { result = invokeNextInterceptor(ctx, prepareCommand); @@ -976,7 +949,7 @@ case Status.STATUS_MARKED_ROLLBACK: case Status.STATUS_ROLLEDBACK: log.debug("Running rollback phase"); - runRollbackPhase(ctx, gtx, tx, modifications); + runRollbackPhase(ctx, gtx, tx); log.debug("Finished rollback phase"); break; @@ -1077,6 +1050,10 @@ modifications = Collections.emptyList(); return; } + else + { + modifications = transactionContext.getModifications(); + } // set any transaction wide options as current for this thread, caching original options that would then be reset originalOptions = ctx.getOptionOverrides(); @@ -1091,7 +1068,6 @@ case Status.STATUS_ACTIVE: case Status.STATUS_PREPARING: // run a prepare call. - modifications = compact(modifications); Object result = isOnePhaseCommit() ? null : runPreparePhase(ctx, gtx, modifications); Modified: core/trunk/src/main/java/org/jboss/cache/invocation/AbstractInvocationContext.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/invocation/AbstractInvocationContext.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/invocation/AbstractInvocationContext.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -38,7 +38,6 @@ private Option optionOverrides; // defaults to true. private boolean originLocal = true; - private boolean txHasMods; private boolean localRollbackOnly; @Deprecated private MethodCall methodCall; @@ -214,20 +213,9 @@ ", transactionContext=" + transactionContext + ", optionOverrides=" + optionOverrides + ", originLocal=" + originLocal + - ", txHasMods=" + txHasMods + '}'; } - public boolean isTxHasMods() - { - return txHasMods; - } - - public void setTxHasMods(boolean b) - { - txHasMods = b; - } - public boolean isLocalRollbackOnly() { return localRollbackOnly; @@ -239,7 +227,6 @@ globalTransaction = null; optionOverrides = null; originLocal = true; - txHasMods = false; invocationLocks = null; methodCall = null; command = null; @@ -256,7 +243,6 @@ copy.originLocal = originLocal; copy.transaction = transaction; copy.transactionContext = transactionContext; - copy.txHasMods = txHasMods; } public void setState(InvocationContext template) @@ -271,7 +257,6 @@ this.setOptionOverrides(template.getOptionOverrides()); this.setOriginLocal(template.isOriginLocal()); this.setTransaction(template.getTransaction()); - this.setTxHasMods(template.isTxHasMods()); } @Override @@ -284,7 +269,6 @@ if (localRollbackOnly != that.localRollbackOnly) return false; if (originLocal != that.originLocal) return false; - if (txHasMods != that.txHasMods) return false; if (globalTransaction != null ? !globalTransaction.equals(that.globalTransaction) : that.globalTransaction != null) { return false; @@ -306,7 +290,6 @@ result = 29 * result + (globalTransaction != null ? globalTransaction.hashCode() : 0); result = 29 * result + (optionOverrides != null ? optionOverrides.hashCode() : 0); result = 29 * result + (originLocal ? 1 : 0); - result = 29 * result + (txHasMods ? 1 : 0); result = 29 * result + (localRollbackOnly ? 1 : 0); return result; } Modified: core/trunk/src/main/java/org/jboss/cache/invocation/InvocationContext.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/invocation/InvocationContext.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/invocation/InvocationContext.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -229,18 +229,6 @@ void setOriginLocal(boolean originLocal); /** - * @return true if the current transaction has any modifications, false otherwise. - */ - boolean isTxHasMods(); - - /** - * Sets whether modifications have been detected on the current transaction. - * - * @param b flag to set - */ - void setTxHasMods(boolean b); - - /** * @return true if the current transaction is set to rollback only. */ boolean isLocalRollbackOnly(); @@ -324,6 +312,5 @@ * @param throwable throwable to throw * @throws Throwable if allowed to throw one. */ - void throwIfNeeded(Throwable throwable - ) throws Throwable; + void throwIfNeeded(Throwable throwable) throws Throwable; } Modified: core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeFactory.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeFactory.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeFactory.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -120,7 +120,7 @@ if ((tctx = ctx.getTransactionContext()) != null) { CreateNodeCommand createNodeCommand = commandsFactory.buildCreateNodeCommand(fqn); - tctx.addModification(createNodeCommand); + tctx.addLocalModification(createNodeCommand); } // notify if we actually created a new child Modified: core/trunk/src/main/java/org/jboss/cache/transaction/MVCCTransactionContext.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/transaction/MVCCTransactionContext.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/transaction/MVCCTransactionContext.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -15,7 +15,7 @@ * @author Manik Surtani (<a href="mailto:manik@jboss.org">manik@jboss.org</a>) * @since 3.0 */ -public class MVCCTransactionContext extends PessimisticTransactionContext +public class MVCCTransactionContext extends AbstractTransactionContext { private final Map<Fqn, NodeSPI> lookedUpNodes = new HashMap<Fqn, NodeSPI>(); Modified: core/trunk/src/main/java/org/jboss/cache/transaction/PessimisticTransactionContext.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/transaction/PessimisticTransactionContext.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/transaction/PessimisticTransactionContext.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -7,20 +7,12 @@ package org.jboss.cache.transaction; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; -import org.jboss.cache.Fqn; import org.jboss.cache.commands.WriteCommand; -import org.jboss.cache.config.Option; -import org.jboss.cache.interceptors.OrderedSynchronizationHandler; -import org.jboss.cache.util.ImmutableListCopy; import javax.transaction.RollbackException; import javax.transaction.SystemException; import javax.transaction.Transaction; -import java.util.ArrayList; import java.util.Collections; -import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; @@ -41,236 +33,43 @@ * @deprecated will be removed along with optimistic and pessimistic locking. */ @Deprecated -public class PessimisticTransactionContext implements TransactionContext +public class PessimisticTransactionContext extends AbstractTransactionContext { + private List<WriteCommand> allModifications; - private static final Log log = LogFactory.getLog(PessimisticTransactionContext.class); - - /** - * Local transaction - */ - private Transaction ltx = null; - private Option option; - private OrderedSynchronizationHandler orderedSynchronizationHandler; - - private boolean forceAsyncReplication = false; - private boolean forceSyncReplication = false; - - /** - * List&lt;ReversibleCommand&gt; of modifications ({@link org.jboss.cache.commands.WriteCommand}). They will be replicated on TX commit - */ - private List<WriteCommand> modificationList; - /** - * A list of modifications that have been encountered with a LOCAL mode option. These will be removed from the modification list during replication. - */ - private List<WriteCommand> localModifications; - - /** - * LinkedHashSet of locks acquired by the transaction. We use a LinkedHashSet because we need efficient Set semantics - * but also need guaranteed ordering for use by lock release code (see JBCCACHE-874). - * <p/> - * This needs to be unchecked since we support both MVCC (Fqns held here) or legacy Opt/Pess locking (NodeLocks held here). - * once we drop support for opt/pess locks we can genericise this to contain Fqns. - Manik Surtani, June 2008 - */ - private LinkedHashSet transactionLocks; - - /** - * A list of dummy uninitialised nodes created by the cache loader interceptor to load data for a - * given node in this tx. - */ - private List<Fqn> dummyNodesCreatedByCacheLoader; - - /** - * List<Fqn> of nodes that have been removed by the transaction - */ - private List<Fqn> removedNodes = null; - public PessimisticTransactionContext(Transaction tx) throws SystemException, RollbackException { - ltx = tx; - orderedSynchronizationHandler = new OrderedSynchronizationHandler(tx); + super(tx); } - public void addModification(WriteCommand command) + @Override + public void addLocalModification(WriteCommand command) { if (command == null) return; - if (modificationList == null) modificationList = new LinkedList<WriteCommand>(); - modificationList.add(command); + super.addLocalModification(command); + if (allModifications == null) allModifications = new LinkedList<WriteCommand>(); + allModifications.add(command); } - public List<WriteCommand> getModifications() - { - if (modificationList == null) return Collections.emptyList(); - return modificationList; - } - - public void addLocalModification(WriteCommand command) - { - if (command == null) throw new NullPointerException("Command is null!"); - if (localModifications == null) localModifications = new LinkedList<WriteCommand>(); - localModifications.add(command); - } - - public List<WriteCommand> getLocalModifications() - { - if (localModifications == null) return Collections.emptyList(); - return localModifications; - } - - - public void addRemovedNode(Fqn fqn) - { - if (fqn == null) throw new NullPointerException("Fqn is null!"); - if (removedNodes == null) removedNodes = new LinkedList<Fqn>(); - removedNodes.add(fqn); - } - - public List<Fqn> getRemovedNodes() - { - if (removedNodes == null) return Collections.emptyList(); - return new ArrayList<Fqn>(removedNodes); - } - - public void setTransaction(Transaction tx) - { - ltx = tx; - } - - public Transaction getTransaction() - { - return ltx; - } - - @SuppressWarnings("unchecked") - public void addLock(Object lock) - { - // no need to worry about concurrency here - a context is only valid for a single thread. - if (transactionLocks == null) transactionLocks = new LinkedHashSet(5); - transactionLocks.add(lock); - } - - @SuppressWarnings("unchecked") - public void removeLock(Object lock) - { - // no need to worry about concurrency here - a context is only valid for a single thread. - if (transactionLocks != null) transactionLocks.remove(lock); - } - - public void clearLocks() - { - if (transactionLocks != null) transactionLocks.clear(); - } - - public boolean hasLock(Object lock) - { - return transactionLocks != null && transactionLocks.contains(lock); - } - - @SuppressWarnings("unchecked") - public void addAllLocks(List newLocks) - { - // no need to worry about concurrency here - a context is only valid for a single thread. - if (transactionLocks == null) transactionLocks = new LinkedHashSet(5); - transactionLocks.addAll(newLocks); - } - - @SuppressWarnings("unchecked") - public List getLocks() - { - return transactionLocks == null || transactionLocks.isEmpty() ? Collections.emptyList() : new ImmutableListCopy(transactionLocks); - } - - - public boolean isForceAsyncReplication() - { - return forceAsyncReplication; - } - - public void setForceAsyncReplication(boolean forceAsyncReplication) - { - this.forceAsyncReplication = forceAsyncReplication; - if (forceAsyncReplication) - { - forceSyncReplication = false; - } - } - - public boolean isForceSyncReplication() - { - return forceSyncReplication; - } - - public void setForceSyncReplication(boolean forceSyncReplication) - { - this.forceSyncReplication = forceSyncReplication; - if (forceSyncReplication) - { - forceAsyncReplication = false; - } - } - - /** - * Returns debug information about this transaction. - */ @Override - public String toString() + public void addModification(WriteCommand command) { - StringBuilder sb = new StringBuilder(); - sb.append("TransactionEntry\nmodificationList: ").append(modificationList); - return sb.toString(); + if (command == null) return; + super.addModification(command); + if (allModifications == null) allModifications = new LinkedList<WriteCommand>(); + allModifications.add(command); } - public void addDummyNodeCreatedByCacheLoader(Fqn fqn) + public List<WriteCommand> getAllModifications() { - if (dummyNodesCreatedByCacheLoader == null) - dummyNodesCreatedByCacheLoader = new LinkedList<Fqn>(); - dummyNodesCreatedByCacheLoader.add(fqn); + if (allModifications == null) return Collections.emptyList(); + return allModifications; } - public List<Fqn> getDummyNodesCreatedByCacheLoader() - { - if (dummyNodesCreatedByCacheLoader == null) return Collections.emptyList(); - return dummyNodesCreatedByCacheLoader; - } - - public void setOption(Option o) - { - this.option = o; - } - - public Option getOption() - { - return this.option; - } - - public OrderedSynchronizationHandler getOrderedSynchronizationHandler() - { - return orderedSynchronizationHandler; - } - - public void setOrderedSynchronizationHandler(OrderedSynchronizationHandler orderedSynchronizationHandler) - { - this.orderedSynchronizationHandler = orderedSynchronizationHandler; - } - - public boolean hasModifications() - { - return modificationList != null && !modificationList.isEmpty(); - } - - public boolean hasLocalModifications() - { - return localModifications != null && !localModifications.isEmpty(); - } - + @Override public void reset() { - orderedSynchronizationHandler = null; - if (modificationList != null) modificationList = null; - if (localModifications != null) localModifications = null; - option = null; - if (transactionLocks != null) transactionLocks.clear(); - if (dummyNodesCreatedByCacheLoader != null) dummyNodesCreatedByCacheLoader.clear(); - if (removedNodes != null) removedNodes.clear(); + super.reset(); + allModifications = null; } } Modified: core/trunk/src/main/java/org/jboss/cache/transaction/TransactionContext.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/transaction/TransactionContext.java 2008-07-31 14:12:50 UTC (rev 6461) +++ core/trunk/src/main/java/org/jboss/cache/transaction/TransactionContext.java 2008-07-31 14:50:35 UTC (rev 6462) @@ -241,7 +241,7 @@ void setOrderedSynchronizationHandler(OrderedSynchronizationHandler orderedSynchronizationHandler); /** - * @return true if modifications were registered to either modificationList or to class loader modifications list. + * @return true if modifications were registered. */ boolean hasModifications(); @@ -251,6 +251,11 @@ boolean hasLocalModifications(); /** + * @return true if either there are modifications or local modifications that are not for replicating. + */ + boolean hasAnyModifications(); + + /** * Cleans up internal state, freeing up references. */ void reset();