geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Jencks <>
Subject Re: [Review] GERONIMO-2277 Remove TransactionContextManager
Date Sat, 05 Aug 2006 00:40:14 GMT
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)

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.

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 checking...

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

Aside from (1) I understand this change and approve it.  I'll apply  
and test it out shortly.

Note that (1) does not affect the functionality, as the sole  
implementations of threadAssociated and threadUnassociated do  
exactly  the same thing.  (1) should not affect applying this patch.

david jencks

On Aug 4, 2006, at 4:14 PM, Dain Sundstrom wrote:

> I have put together a patch from the notcm branch, and attached it to:
> The best way to test this chang is to use svn merge directly  
> instead of using the patch commnd. This is because the svn merge  
> handles adding, removing and relocating files. To merge the changes  
> from the notcm branch execute the following command from within  
> geronimo/trunk
>   svn merge -r 427246:HEAD 
> geronimo/branches/dain/notcm .
> You should not receive any conflicts during the merge. Once this is  
> complete, run the following commands to remove the existing openejb  
> checkout and checkout the new related openejb code (this will be  
> merged directly in openejb):
>   rm -rf openejb/
>   maven m:co
> Then simply run the maven build with the following command:
>   maven install
> Altermatively, you can use the attached patch insted of the svn  
> merge command.
> Please, take a look at this sooner rather than later as the TCM  
> touched lots of code, so this is a large patch.  Currently, it  
> applies without conflict but that could easily change.
> Thanks in advance,
> -dain

View raw message