Mailing-List: contact dev-help@geronimo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@geronimo.apache.org
Received-SPF: pass (asf.osuosl.org: local policy)
Mime-Version: 1.0 (Apple Message framework v752.2)
In-Reply-To: <0DE6BC17-808F-4B52-9B13-7DB7538D4663@yahoo.com>
References: <2760C2D9-F4AF-40E3-AAAF-ECCBE2B5EED5@iq80.com>
 <0DE6BC17-808F-4B52-9B13-7DB7538D4663@yahoo.com>
Content-Type: text/plain; charset=US-ASCII; delsp=yes; format=flowed
Message-Id: <57593E8A-C9CF-4DB6-BB63-D5DA71A0044D@iq80.com>
Content-Transfer-Encoding: 7bit
From: Dain Sundstrom <dain@iq80.com>
Subject: Re: [Review] GERONIMO-2277 Remove TransactionContextManager
Date: Sat, 5 Aug 2006 21:11:14 -0700
To: dev@geronimo.apache.org

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