cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Huang" <alex.hu...@citrix.com>
Subject Re: Review Request: Handle self-managed db connection correctly
Date Thu, 25 Oct 2012 16:43:48 GMT


> On Oct. 25, 2012, 2:59 a.m., Alex Huang wrote:
> > Nicely written unit test.  You should add one to test what happens when the transaction
is reused.  For every bug we find, we should write a unit test for it.  Also document in the
unit test what you are testing for so other people who read the unit test understands why
it is necessary.
> > 
> > Thanks.
> > 
> > Also I think the transition should happen before the txn closed for symmetry with
the txn open and then transit to user managed.
> 
> Min Chen wrote:
>     Regarding ordering transitToAutoManagedConnection call before txn.close(), I am not
quite convinced. Since txn.close() has some code to do rollback in removeUpTo subroutine,
which is directly invoke _conn.rollback. If we invoke transitToAutoManagedConnection first,
we may encounter null pointer exception.

That's actually good.  By the time you get to that last txn.close(), the stack better be rolled
up already or someone actually messed up the stack so you want this exception to surface.
 You can write some unit tests to ensure that the behavior is correct.  Remember the transitToUserManaged
ensures via assert that there's a clean stack to begin with.

Don't think with what you know about the Transaction code.  Think as if you're the user of
the code.  

Transaction.open()
transitToUserManaged()
transitToAutoManaged()
Transaction.close()

A flow like that makes much more sense.  If the Transaction code doesn't handle that right,
then it's the Transaction code that's incorrect.


- Alex


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7726/#review12754
-----------------------------------------------------------


On Oct. 25, 2012, 12:02 a.m., Min Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7726/
> -----------------------------------------------------------
> 
> (Updated Oct. 25, 2012, 12:02 a.m.)
> 
> 
> Review request for cloudstack and Alex Huang.
> 
> 
> Description
> -------
> 
> This patch is to fix this bug https://issues.apache.org/jira/browse/CLOUDSTACK-409
> 
> 
> This addresses bug CLOUDSTACK-409.
> 
> 
> Diffs
> -----
> 
>   server/src/com/cloud/api/ApiServlet.java 8a1d4de 
>   server/src/com/cloud/api/response/ApiResponseSerializer.java 1429d14 
>   server/src/com/cloud/cluster/ClusterManagerImpl.java 4dbb16c 
>   utils/src/com/cloud/utils/db/Transaction.java bcf7ae1 
>   utils/test/com/cloud/utils/db/DbTestDao.java PRE-CREATION 
>   utils/test/com/cloud/utils/db/DbTestVO.java PRE-CREATION 
>   utils/test/com/cloud/utils/db/TransactionTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/7726/diff/
> 
> 
> Testing
> -------
> 
> Testing is done locally.
> 
> 
> Thanks,
> 
> Min Chen
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message