|
The patch looks good to me. I'll run the tests and commit it.
Committed d2551.diff to trunk with revision 531468 and to 10.2 with revision 531470.
can this issue be resolved, looks like patch went in.
I would like to provide also a test for the fix for trunk only. I'll do that in following days.
The patch contains the junit test for the trunk.
I ran derbyall and suites.All without failures. Hi Julo,
The test looks good, but I have a couple of small comments: 1) The new file (XATransactionTest.java) is tab free with the exception of two lines. It would be great if you could remove the tabs from them as well. 2) The test runs in both embedded and client/server mode, but it always uses ClientXid. Is that OK? 3) If parseXid() used assertNotNull(str) and assertTrue(str.matches(...)) instead of returning null, it would be easier to see the problem if the test fails. 4) Assert.assertTrue(xid.equals(rXid)) could be replaced with Assert.assertEquals(xid, rXid), which provides more information if the test fails. 5) rs, stm and conn aren't closed. 6) Using finally clauses to do clean-up might hide the actual error. If you remove try/finally, it will still do the clean-up when the test runs successfully (which it is supposed to do). If you think it's really needed to have the clean-up in a finally block, I would recommend moving it to the tearDown() method. Thanks, Knut!
1, 3, 4, 6) Thanks, I'll fix theese. 2.) Using the ClientXid is ok since it only stores the transaction identification and does not contain any client driver related code. There is no Xid interface implementation for the embedded driver. 5.) Shouldn't this happen when they got garbage collected? I have consulted the javadoc for Statement.close and Connection.close methods and prior to version 1.6 they should be closed when they get garbage collected. In version 1.6 this sentence disappeared. Shouldn't we write the tests more sloppy that also a garbage collection would be tested? > Using the ClientXid is ok since it only stores the transaction identification and does not contain any client driver related code.
Sounds OK. Perhaps you could add a comment about that in the test. > Shouldn't we write the tests more sloppy that also a garbage collection would be tested? Definitely not! :) We have already tried that, and it only made the JUnit tests fail intermittently and randomly. The most common problem is that tearDown() is not allowed to drop a table because some open object depends on the table and hasn't been garbage collected yet. I think there are some tests for memory leaks which implicitly test that garbage collection work. I changed finally to catch with throwing the same exception further even if another one occurs (the new one would be ignored since it is not the one that failed the test). Is it ok to do it that way?I need to call at least rollback on that transaction because the CleanDatabaseSetup will not be able to clean the database because the transaction will keep holding locks.
Thanks Julo! The latest patch looks good. Committed revision 538770.
I forgot one thing in the review. I think the test needs to be disabled under J2ME since it uses XADataSource. The attached patch moves the test into the JDBC 3 section of jdbcapi._Suite and also makes the test itself check the JDBC version in its suite().
Committed revision 538818. Thanks, Knut. I did not know that something like your j2me patch is necessary.
Thaks again. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
Global Xid value garbled in syscs_diag.transaction_table.
I ran derbyall and suites.All without failures.
Please, review the patch and if you find it to be ok, please apply it to trunk and 10.2 branch also. I will provide an additional patch with a junit test for trunk only.