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