geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dain Sundstrom <d...@iq80.com>
Subject Re: [Review] GERONIMO-2277 Remove TransactionContextManager
Date Sun, 06 Aug 2006 04:11:14 GMT
I merged all changes from head back into notcm and fixed the m2  
build.  I also made a few changes noted below based on requests for  
David.  You I suggest you test the notcm branch directly.  If you  
want to try the merge, use the same command as before, and you'll  
have to resolve some conflicts (I have no idea why svn marks them as  
conflicts).  Here are the commands I used:

svn co https://svn.apache.org/repos/asf/geronimo/trunk geronimo
cd geronimo

svn merge -r 427246:HEAD https://svn.apache.org/repos/asf/geronimo/ 
branches/dain/notcm .

# resolve the conflicts by taking the files from notcm
mv bootstrap.merge-right* \
    bootstrap
svn resolved bootstrap
chmod u+x bootstrap

mv pom.xml.merge-right* \
    pom.xml
svn resolved pom.xml

mv m2-assemblies/geronimo-boilerplate-j2ee/src/main/resources/var/ 
security/keystores/geronimo-default.merge-right* \
    m2-assemblies/geronimo-boilerplate-j2ee/src/main/resources/var/ 
security/keystores/geronimo-default
svn resolved m2-assemblies/geronimo-boilerplate-j2ee/src/main/ 
resources/var/security/keystores/geronimo-default

mv m2-assemblies/geronimo-boilerplate-minimal/src/main/resources/var/ 
security/keystores/geronimo-default.merge-right* \
    m2-assemblies/geronimo-boilerplate-minimal/src/main/resources/var/ 
security/keystores/geronimo-default
svn resolved m2-assemblies/geronimo-boilerplate-minimal/src/main/ 
resources/var/security/keystores/geronimo-default

# run the m2 build or run the m1 build
./bootstrap
mvn clean install

# maven new

Sorry about the long instructions, but svn screwed me. more comments  
below...


On Aug 4, 2006, at 5:40 PM, David Jencks wrote:

> I have a couple of concerns: most of these were probably present in  
> the unmodified code.
>
> 1. TransactionManagerImpl.unassociate calls fireThreadAssociated  
> instead of fireThreadUnassociated (dain is fixing this IIUC)

FIXED

> 2. ConnectorTransactionContext.managedConnections doesn't have any  
> obvious synchronization.  When I wrote this code I think I had an  
> argument why it wasn't needed, but I'd like to find the object that  
> is in fact guarding it and document it.

I synchronized access.  It the data is effectively transaction local,  
and only thread can be associated with a tx at a time. But it is  
safer to synchronize.  It shouldn't hurt performance since there will  
be no contention.

> 3. Am I correct in thinking that you made the connection tracking  
> work without needing instance contexts available?  I think this is  
> a reasonable change, just

Based on my reading of the code, conection tracking need to be  
notified of context changes.  If it switches to a context without  
connector tx data, the tracker has nothing to do.

> 4. I wonder if the client still needs a tm since it is inaccessible.

I don't think it is requires, but we may want to start an  
unrecoverable one anyway.

-dain


Mime
View raw message