Derby
  1. Derby
  2. DERBY-4653

Avoid unnecessary round-trip for commit in the client driver

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.7.1.1
    • Fix Version/s: 10.7.1.1
    • Component/s: JDBC, Network Client
    • Labels:
      None
    • Issue & fix info:
      Patch Available
    • Bug behavior facts:
      Performance

      Description

      The methods Connection.commit() and Connection.rollback() in the client driver cause a round-trip to the server even if the commit/rollback is unnecessary (i.e. there is nothing to commit or roll back).
      Comments suggest (see below) that this can be optimized, such that the commands are flowed to the server only when required. It can be seen that this optimization has been used other places in the client driver. Never the less, it must be checked that this optimization doesn't have side-effects.

      This issue came up in connection with connection pooling, where a pool implementation always issued a rollback to make sure there was no active transaction on the connection handed out.

      From Connection.flowCommit:
      // Per JDBC specification (see javadoc for Connection.commit()):
      // "This method should be used only when auto-commit mode has been disabled."
      // However, some applications do this anyway, it is harmless, so
      // if they ask to commit, we could go ahead and flow a commit.
      // But note that rollback() is less harmless, rollback() shouldn't be used in auto-commit mode.
      // This behavior is subject to further review.

      // if (!this.inUnitOfWork)
      // return;
      // We won't try to be "too smart", if the user requests a commit, we'll flow a commit,
      // regardless of whether or not we're in a unit of work or in auto-commit mode.
      //

      1. DERBY-4653-1.diff
        0.9 kB
        Lily Wei
      2. DERBY-4653-2.diff
        2 kB
        Lily Wei
      3. SaveRoundClientDS.java
        4 kB
        Lily Wei
      4. _sds_0
        95 kB
        Lily Wei
      5. DERBY-4653-3_withrollback.diff
        3 kB
        Lily Wei
      6. SaveRoundClientDS.java
        4 kB
        Lily Wei
      7. DERBY-4653-4_withcommitrollbacktest.diff
        7 kB
        Lily Wei
      8. DERBY-4653-5_withflowcommitrollback.diff
        8 kB
        Lily Wei
      9. DERBY-4653-6_withflowcommitrollback.diff
        9 kB
        Lily Wei
      10. ReproTransInProgressAttempt.java
        3 kB
        Lily Wei
      11. DERBY-4653-7_withflowcommittest.diff
        6 kB
        Lily Wei
      12. DERBY-4653-7_withflowcommittest_comment_update_diff.txt
        6 kB
        Kathey Marsden
      13. DERBY-4653-7_withflowcommittest_comment_update_diff.txt
        6 kB
        Kristian Waagan
      14. DERBY-4653-7_withflowcommittest_comment_update_diff_followup.txt
        2 kB
        Lily Wei

        Issue Links

          Activity

          Hide
          Lily Wei added a comment -

          Change the code in Connection.flowcommit() to save the round trip
          if (!this.inUnitOfWork_)
          return;

          Run suites.AllPackages and test_04_undefinedAndIllegal and test_04_undefinedAndIllegal fails.
          1) test_04_undefinedAndIllegal(org.apache.derbyTesting.functionTests.tests.lang.Bo
          oleanValuesTest)junit.framework.ComparisonFailure: getBoolean() on BLOB_COL expect
          ed:<...2005> but was:<...4000>
          at org.apache.derbyTesting.junit.BaseJDBCTestCase.assertSQLState(BaseJDBCT
          estCase.java:762)
          at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.vet_
          getBooleanException(BooleanValuesTest.java:430)
          at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.vet_
          getBooleanIsIllegal(BooleanValuesTest.java:410)
          at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.test
          _04_undefinedAndIllegal(BooleanValuesTest.java:332)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.ja
          va:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccesso
          rImpl.java:25)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:10
          9)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:19)
          at junit.extensions.TestSetup.run(TestSetup.java:23)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          Caused by: java.sql.SQLException: Invalid cursor state - no current row.

          However, these two tests fail when I don't have Connect.flowcommit() change either.

          On BooleanTestsValue.java,
          public void test_04_undefinedAndIllegal() throws Exception
          {
          Connection conn = getConnection();

          vet_getBooleanIsIllegal( conn, "BLOB_COL" ); <<<====This is the failure comes from

          I will keep looking to see why these two tests failed with or without Connection.flowcommit(() change.

          Show
          Lily Wei added a comment - Change the code in Connection.flowcommit() to save the round trip if (!this.inUnitOfWork_) return; Run suites.AllPackages and test_04_undefinedAndIllegal and test_04_undefinedAndIllegal fails. 1) test_04_undefinedAndIllegal(org.apache.derbyTesting.functionTests.tests.lang.Bo oleanValuesTest)junit.framework.ComparisonFailure: getBoolean() on BLOB_COL expect ed:<...2005> but was:<...4000> at org.apache.derbyTesting.junit.BaseJDBCTestCase.assertSQLState(BaseJDBCT estCase.java:762) at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.vet_ getBooleanException(BooleanValuesTest.java:430) at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.vet_ getBooleanIsIllegal(BooleanValuesTest.java:410) at org.apache.derbyTesting.functionTests.tests.lang.BooleanValuesTest.test _04_undefinedAndIllegal(BooleanValuesTest.java:332) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.ja va:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccesso rImpl.java:25) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:10 9) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:22) at junit.extensions.TestSetup$1.protect(TestSetup.java:19) at junit.extensions.TestSetup.run(TestSetup.java:23) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) Caused by: java.sql.SQLException: Invalid cursor state - no current row. However, these two tests fail when I don't have Connect.flowcommit() change either. On BooleanTestsValue.java, public void test_04_undefinedAndIllegal() throws Exception { Connection conn = getConnection(); vet_getBooleanIsIllegal( conn, "BLOB_COL" ); <<<====This is the failure comes from I will keep looking to see why these two tests failed with or without Connection.flowcommit(() change.
          Hide
          Lily Wei added a comment -

          Other than the two failure tests, suites.AllPackages and derbyall passed with DERBY-4653-1.diff.

          Show
          Lily Wei added a comment - Other than the two failure tests, suites.AllPackages and derbyall passed with DERBY-4653 -1.diff.
          Hide
          Lily Wei added a comment -

          I notice with java 1.6.0_13, test BooleanValuesTest does not run.
          Is this expected behavior?

          $ java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang.B
          ooleanValuesTest

          Time: 0.001

          OK (0 tests)

          $ java -version
          java version "1.6.0_13"
          Java(TM) SE Runtime Environment (build 1.6.0_13-b03)
          Java HotSpot(TM) Client VM (build 11.3-b02, mixed mode)

          Show
          Lily Wei added a comment - I notice with java 1.6.0_13, test BooleanValuesTest does not run. Is this expected behavior? $ java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang.B ooleanValuesTest Time: 0.001 OK (0 tests) $ java -version java version "1.6.0_13" Java(TM) SE Runtime Environment (build 1.6.0_13-b03) Java HotSpot(TM) Client VM (build 11.3-b02, mixed mode)
          Hide
          Knut Anders Hatlen added a comment -

          Hi Lily,

          The problems with BooleanValuesTest were fixed in DERBY-4674. You probably ran with java 1.6.0_13 after my initial check-in that disabled the test if Xalan was not available. Rick later re-enabled the test and made it run cleanly on platforms that didn't have the required XML libraries, so I believe it should be running fine now if you do an svn update first.

          Show
          Knut Anders Hatlen added a comment - Hi Lily, The problems with BooleanValuesTest were fixed in DERBY-4674 . You probably ran with java 1.6.0_13 after my initial check-in that disabled the test if Xalan was not available. Rick later re-enabled the test and made it run cleanly on platforms that didn't have the required XML libraries, so I believe it should be running fine now if you do an svn update first.
          Hide
          Lily Wei added a comment -

          Hi Knut:
          Thank you so much for the prompt response. I did svn update and up to revision 948072 and upgrade my jdk to 1.6.0_20.
          For some reasons, the test in BooleanValuesTest is not running.

          $ java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang.B
          ooleanValuesTest
          ..........
          Time: 8.245

          OK (10 tests)

          $ java -version
          java version "1.6.0_20"
          Java(TM) SE Runtime Environment (build 1.6.0_20-b02)
          Java HotSpot(TM) Client VM (build 16.3-b01, mixed mode, sharing)

          $ svn update
          At revision 948072.

          Show
          Lily Wei added a comment - Hi Knut: Thank you so much for the prompt response. I did svn update and up to revision 948072 and upgrade my jdk to 1.6.0_20. For some reasons, the test in BooleanValuesTest is not running. $ java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang.B ooleanValuesTest .......... Time: 8.245 OK (10 tests) $ java -version java version "1.6.0_20" Java(TM) SE Runtime Environment (build 1.6.0_20-b02) Java HotSpot(TM) Client VM (build 16.3-b01, mixed mode, sharing) $ svn update At revision 948072.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Lily. From the output you posted above, it looks like the test is running successfully (it says OK - 10 tests). Or is there some other problem you are seeing?

          Show
          Knut Anders Hatlen added a comment - Hi Lily. From the output you posted above, it looks like the test is running successfully (it says OK - 10 tests). Or is there some other problem you are seeing?
          Hide
          Lily Wei added a comment -

          Hi Knut:
          You are right. It does say 10 tests. I was hoping to see the list of the 10 tests. However, it may not be necessary. Thanks.

          Show
          Lily Wei added a comment - Hi Knut: You are right. It does say 10 tests. I was hoping to see the list of the 10 tests. However, it may not be necessary. Thanks.
          Hide
          Kristian Waagan added a comment -

          Hi Lily,

          If you want to see the test names, you can run with the Swing (or awt) test runner [1], or specify the trace property [2] understood by the Derby test framework.

          [1] junit.swingui.TestRunner -noloading
          [2] -Dderby.tests.trace=true

          Show
          Kristian Waagan added a comment - Hi Lily, If you want to see the test names, you can run with the Swing (or awt) test runner [1] , or specify the trace property [2] understood by the Derby test framework. [1] junit.swingui.TestRunner -noloading [2] -Dderby.tests.trace=true
          Hide
          Lily Wei added a comment -

          Thank you so much, Kristian. That is such a good suggestion. I will give it a try.

          I add the patch for Connection.flowcommit() so it won't flow if we don't need to perform the unnecessary round trip.
          Please see DERBY-4653-2.diff. I also try to add the test for testing saving commit. I add an extra commit on J2EEDataSourceTest to test the fix. However, if the behavior go back to the round-trip scenario, the test has no way knowing other than functionality is working. Does anyone has a good suggest to test the DRDA protocol for commit making round trip or not?

          About making Connection.flowrollback not doing the round trip, It is not as straight forward as detecting isUnitofWork_ only. I think the check has to happen along with isXAConnection_ and where we are in the rollback state. I will keep working on that today. However, I do think that it is safe to commit DERBY-4653-2.diff. I will suggest to open a new JIRA for rollback only depend on how to test Connection.flowcommit DRDA related protocol.

          Show
          Lily Wei added a comment - Thank you so much, Kristian. That is such a good suggestion. I will give it a try. I add the patch for Connection.flowcommit() so it won't flow if we don't need to perform the unnecessary round trip. Please see DERBY-4653 -2.diff. I also try to add the test for testing saving commit. I add an extra commit on J2EEDataSourceTest to test the fix. However, if the behavior go back to the round-trip scenario, the test has no way knowing other than functionality is working. Does anyone has a good suggest to test the DRDA protocol for commit making round trip or not? About making Connection.flowrollback not doing the round trip, It is not as straight forward as detecting isUnitofWork_ only. I think the check has to happen along with isXAConnection_ and where we are in the rollback state. I will keep working on that today. However, I do think that it is safe to commit DERBY-4653 -2.diff. I will suggest to open a new JIRA for rollback only depend on how to test Connection.flowcommit DRDA related protocol.
          Hide
          Lily Wei added a comment -

          The patch pass suites.Allpackages and derbyall test suites.

          Show
          Lily Wei added a comment - The patch pass suites.Allpackages and derbyall test suites.
          Hide
          Kristian Waagan added a comment -

          Lily wrote:


          However, if the behavior go back to the round-trip scenario, the test has no way knowing other than functionality is working. Does anyone has a good suggest to test the DRDA protocol for commit making round trip or not?


          A few suggestions (none of them really optimal):

          • add different trace points on the client for the cases where a commit is flowed or not, then parse the trace file
            (this change could actually help debugging in production environments)
          • enable statement logging on the server and parse the log looking for the number of commits (assuming an "unnecessary" commit produces a log message)
          • some kind of timed test doing as many commits as possible (hard to set the thresholds, probably not a viable solution?)
          • add special debug code, or exploit poor state encapsulation in the client driver
            (for instance, can the DRDA "sequence number" be used, or more likely, maybe the client side "transaction id"?)

          Another related task would be to inspect the code to make sure the variable 'inUnitOfWork_' is in sync with reality.

          Show
          Kristian Waagan added a comment - Lily wrote: However, if the behavior go back to the round-trip scenario, the test has no way knowing other than functionality is working. Does anyone has a good suggest to test the DRDA protocol for commit making round trip or not? A few suggestions (none of them really optimal): add different trace points on the client for the cases where a commit is flowed or not, then parse the trace file (this change could actually help debugging in production environments) enable statement logging on the server and parse the log looking for the number of commits (assuming an "unnecessary" commit produces a log message) some kind of timed test doing as many commits as possible (hard to set the thresholds, probably not a viable solution?) add special debug code, or exploit poor state encapsulation in the client driver (for instance, can the DRDA "sequence number" be used, or more likely, maybe the client side "transaction id"?) Another related task would be to inspect the code to make sure the variable 'inUnitOfWork_' is in sync with reality.
          Hide
          Dag H. Wanvik added a comment -

          Maybe you can trace the protocol and check the trace file contents, cf.
          http://db.apache.org/derby/docs/10.6/adminguide/cadminappsclienttracing.html

          Show
          Dag H. Wanvik added a comment - Maybe you can trace the protocol and check the trace file contents, cf. http://db.apache.org/derby/docs/10.6/adminguide/cadminappsclienttracing.html
          Hide
          Lily Wei added a comment -

          Thanks Dag and Kristian.
          I did use ClientDataSource and ClientDataSource.setTraceDirectory to see that the change for save round trip for the commit. I manually reading the trace file "_sds_0" to see the second commit() does not have that many trace information like SEND BUFFER: RDBCMN ... ENDDUOWRM ... Thanks Kathey for helping me understand those complicated message. There is SaveRoundClientDS.java and trace file "_sds_0" in the attachment to show the test code and trace information. To parse the trace file in the test suite, is there something available already? Can someone elaborate on this idea?

          Show
          Lily Wei added a comment - Thanks Dag and Kristian. I did use ClientDataSource and ClientDataSource.setTraceDirectory to see that the change for save round trip for the commit. I manually reading the trace file "_sds_0" to see the second commit() does not have that many trace information like SEND BUFFER: RDBCMN ... ENDDUOWRM ... Thanks Kathey for helping me understand those complicated message. There is SaveRoundClientDS.java and trace file "_sds_0" in the attachment to show the test code and trace information. To parse the trace file in the test suite, is there something available already? Can someone elaborate on this idea?
          Hide
          Kristian Waagan added a comment -

          Would it be good enough to simply iterate over the lines in the trace file, counting occurrences of RDBCMM and RDBRLLBCK?

          I modified your repro, adding some more commits and rollbacks. When doing a manual grep on the trace file, I got 50 matches without your patch, and 17 with.
          To parse the file you should use SupportFilesSetup and PrivilegedFileOpsForTests.

          Show
          Kristian Waagan added a comment - Would it be good enough to simply iterate over the lines in the trace file, counting occurrences of RDBCMM and RDBRLLBCK? I modified your repro, adding some more commits and rollbacks. When doing a manual grep on the trace file, I got 50 matches without your patch, and 17 with. To parse the file you should use SupportFilesSetup and PrivilegedFileOpsForTests.
          Hide
          Kathey Marsden added a comment -

          I think that sounds like a good idea to check the file for the RDBCMM string and to make the name more predictable, you might want to use traceFile instead of traceDirectory. Also I think it makes sense to check in the commit part of this patch while working through the rollback issues in another patch, but it would be good to do so with the regression test for the commit portion.

          Show
          Kathey Marsden added a comment - I think that sounds like a good idea to check the file for the RDBCMM string and to make the name more predictable, you might want to use traceFile instead of traceDirectory. Also I think it makes sense to check in the commit part of this patch while working through the rollback issues in another patch, but it would be good to do so with the regression test for the commit portion.
          Hide
          Kristian Waagan added a comment -

          Removed unused method Connection.setInUnitOfWork(boolean) with revision 950185.

          Show
          Kristian Waagan added a comment - Removed unused method Connection.setInUnitOfWork(boolean) with revision 950185.
          Hide
          Lily Wei added a comment -

          Thanks to Kristian and Kathey.
          I fix the avoid traffic for flowrollback(). We need to just flow rollback when we are in XAConnection.
          The patch passed suites.allpackages and derbyall.

          To test the fix:
          1. Will it be okay if we just reflect the method in ..am.Connection.getTransactionID() and make sure the next transaction is after first commit()()/rollback is a predictable number. After the test is run, there will always be a trace file hanging around for manual inspection. It could be cheating a little bit. However, it is a little easier than test open the trace file and search for the number of occurrences of RDBCMM or something like that.

          Show
          Lily Wei added a comment - Thanks to Kristian and Kathey. I fix the avoid traffic for flowrollback(). We need to just flow rollback when we are in XAConnection. The patch passed suites.allpackages and derbyall. To test the fix: 1. Will it be okay if we just reflect the method in ..am.Connection.getTransactionID() and make sure the next transaction is after first commit()()/rollback is a predictable number. After the test is run, there will always be a trace file hanging around for manual inspection. It could be cheating a little bit. However, it is a little easier than test open the trace file and search for the number of occurrences of RDBCMM or something like that.
          Hide
          Lily Wei added a comment -

          The fix is the same as before, we flowcommit() when it is safe and we flowrollback() when we are in XAConnection. I run it against Suites.All and Derbyall and they all passed.

          With Kathey's help, I add new test to test just Connection.flowcommit() and Connection.flowrollback(). The different signature testConnectionFlowCommitRollback() is added to J2EEDataSourceTest.

          In this test, when we flow commit, we check the transaction increase by 1.
          When we are testing rollback, we rollback the transaction and execute an insert statement, the transaction increase by 2.

          With the little repro, manual check the trace file to see the DRDA protocol trace save the round trip for flowcommit() and flowrollback() is also done.

          Show
          Lily Wei added a comment - The fix is the same as before, we flowcommit() when it is safe and we flowrollback() when we are in XAConnection. I run it against Suites.All and Derbyall and they all passed. With Kathey's help, I add new test to test just Connection.flowcommit() and Connection.flowrollback(). The different signature testConnectionFlowCommitRollback() is added to J2EEDataSourceTest. In this test, when we flow commit, we check the transaction increase by 1. When we are testing rollback, we rollback the transaction and execute an insert statement, the transaction increase by 2. With the little repro, manual check the trace file to see the DRDA protocol trace save the round trip for flowcommit() and flowrollback() is also done.
          Hide
          Kristian Waagan added a comment -

          Sorry for the short post (it's late here now), but I think there are some issues with the patch.
          Feel free to have another look at it right away, I'll have a look on Monday in any case.

          Also, I'll be happy to help you write the code parsing the trace file (it's good to keep the existing test) , i.e. something like:

          • obtain first connection, run test sequence without redundant commits
          • parse trace file to obtain baseline for number of commits
          • obtain second connection, run test sequence with many of extra commits
          • parse trace file to obtain number of commits
          • compare the commit counts

          In this case it might be good to factor out the test sequence code into a method with a boolean telling wheter or not to do the extra commits.
          It's fine if you don't implement this test, I may have a go at it if you don't

          Show
          Kristian Waagan added a comment - Sorry for the short post (it's late here now), but I think there are some issues with the patch. Feel free to have another look at it right away, I'll have a look on Monday in any case. Also, I'll be happy to help you write the code parsing the trace file (it's good to keep the existing test) , i.e. something like: obtain first connection, run test sequence without redundant commits parse trace file to obtain baseline for number of commits obtain second connection, run test sequence with many of extra commits parse trace file to obtain number of commits compare the commit counts In this case it might be good to factor out the test sequence code into a method with a boolean telling wheter or not to do the extra commits. It's fine if you don't implement this test, I may have a go at it if you don't
          Hide
          Lily Wei added a comment -

          Thank you so much, Kristian. It is late over there. We are so international. Not a problem about the short post. I was not totally clear in turn of the issues with the patch.

          It will be great to have your help on code parsing and run test sequence with all the necessary steps for commits. This will be more tests on top of the existing test. And, it will be very complete tests.

          Show
          Lily Wei added a comment - Thank you so much, Kristian. It is late over there. We are so international. Not a problem about the short post. I was not totally clear in turn of the issues with the patch. It will be great to have your help on code parsing and run test sequence with all the necessary steps for commits. This will be more tests on top of the existing test. And, it will be very complete tests.
          Hide
          Kristian Waagan added a comment -

          Hi Lily,

          Here are my comments on patch 'DERBY-4653-4_withcommitrollbacktest.diff':
          — J2EEDataSourceTest
          1) The test should be added to the client suite, since it is only relevant for the client driver.
          2) The method comment could well be changed to a JavaDoc comment (/* -> /**)
          3) Can the logic for skipping the test (i.e. wheter Connection.getTransactionID exists)
          be moved into the suite method?)
          If you create a static method to check if the method exists, if can be used both in the
          suite method and in J2EEDataSourceTest.getConnectionID. There is one complication, and that is
          that you cannot (or should not) obtain a connection in the suite method. Is passing the ClientDriver
          class good enough?
          If this gets too messy, just let it be. My concern with short-circuiting the tests themselves, is that
          the will be listed as run and passed, even though they haven't really been run.
          Does anyone have any options on this? Have we solved this problem earlier?
          4) Instead of checking for a specific number of rows (X -14 >=0) ) in the system table
          (which we don't really know), is it good enough to assert
          rs.next() == true, rs.getInt() >= 0, rs.next() == false ?

          — Connection
          5) Wrong comment? If we are in a connection, we do want to commit, right?
          6) Can you explain the reasoning behind the changes in flowRollback?
          I don't understand them, as I'm not that familiar with XA.

          — LogicalConnection
          7) I don't understand the comment about embedded, nor why we have to add this method.
          If it is to enable testing with logical connections (i.e. with XADataSource or CPDataSource),
          maybe it is better to parse the trace file? In any case it would be nice to test a
          "normal connection", since there is some special code for when the connection is originating
          from CP-|XADataSource.

          Possible improvements:
          a) Add test parsing the trace file.
          b) Test with XA as well (since there is special code for it).

          Show
          Kristian Waagan added a comment - Hi Lily, Here are my comments on patch ' DERBY-4653 -4_withcommitrollbacktest.diff': — J2EEDataSourceTest 1) The test should be added to the client suite, since it is only relevant for the client driver. 2) The method comment could well be changed to a JavaDoc comment (/* -> /**) 3) Can the logic for skipping the test (i.e. wheter Connection.getTransactionID exists) be moved into the suite method?) If you create a static method to check if the method exists, if can be used both in the suite method and in J2EEDataSourceTest.getConnectionID. There is one complication, and that is that you cannot (or should not) obtain a connection in the suite method. Is passing the ClientDriver class good enough? If this gets too messy, just let it be. My concern with short-circuiting the tests themselves, is that the will be listed as run and passed, even though they haven't really been run. Does anyone have any options on this? Have we solved this problem earlier? 4) Instead of checking for a specific number of rows (X -14 >=0) ) in the system table (which we don't really know), is it good enough to assert rs.next() == true, rs.getInt() >= 0, rs.next() == false ? — Connection 5) Wrong comment? If we are in a connection, we do want to commit, right? 6) Can you explain the reasoning behind the changes in flowRollback? I don't understand them, as I'm not that familiar with XA. — LogicalConnection 7) I don't understand the comment about embedded, nor why we have to add this method. If it is to enable testing with logical connections (i.e. with XADataSource or CPDataSource), maybe it is better to parse the trace file? In any case it would be nice to test a "normal connection", since there is some special code for when the connection is originating from CP-|XADataSource. Possible improvements: a) Add test parsing the trace file. b) Test with XA as well (since there is special code for it).
          Hide
          Kathey Marsden added a comment - - edited

          wrt to some of Kristian's comments and a few things I talked to Lily about today.

          I too think it would be good to test just for client and with regular, pooled, and xa datasources.

          I think if the test runs just for client we should always count on the getTransactionID() method being there and can take out the skipping logic all together. I think the system table query can come out all together. I tend to think the getTransactionID() check is good enough, but the protocol trace parsing would be ok too.

          I agree the comment about embedded should come out of am.LogicalConnection.

          For the flowRollback() change I think the condition to return if not inUnitOfWork should not be in the if block. I think the StatementPoolingTest failures you were seeing with it set that way may be because the state of inUnitOfWork may be wrong somehow where hold cursors and pooled connections are involved, probably an existing issue that needs to be tracked down before committing this change. Lily and I tried to reproduce it outside the test with the code but we were not quite able to get in the same state as StatementPoolingTest.

          Show
          Kathey Marsden added a comment - - edited wrt to some of Kristian's comments and a few things I talked to Lily about today. I too think it would be good to test just for client and with regular, pooled, and xa datasources. I think if the test runs just for client we should always count on the getTransactionID() method being there and can take out the skipping logic all together. I think the system table query can come out all together. I tend to think the getTransactionID() check is good enough, but the protocol trace parsing would be ok too. I agree the comment about embedded should come out of am.LogicalConnection. For the flowRollback() change I think the condition to return if not inUnitOfWork should not be in the if block. I think the StatementPoolingTest failures you were seeing with it set that way may be because the state of inUnitOfWork may be wrong somehow where hold cursors and pooled connections are involved, probably an existing issue that needs to be tracked down before committing this change. Lily and I tried to reproduce it outside the test with the code but we were not quite able to get in the same state as StatementPoolingTest.
          Hide
          Lily Wei added a comment - - edited

          Thanks Kathey for showing me around and making this patch to this stage and explain all the issues to me.

          With this patch, it has the intention fix for Connection.flowcommit() and Connection.flowrollback(). However, when running suites.All, some tests failed. i.e. jdbcapi.StatementPollingTest

          The change of the patch which answer the issue Kristian point out. Thanks Kristian for the looking at the patch and beyond.

          J2EEDataSourceTest
          1. The test gets added to getClientSuite(). The test is only running on regular client, pooled, and xa datasources.
          2. Change to javadoc as Kristian point out.
          3. Please see comment in BaseJdbcTestCase.java
          4. No need to use the system table as part of test anymore.
          BaseJdbcTestCase.java
          5. getClientTransactonID(Connection conn) is in BaseJdbcTestCase.java so it is closer in suites than J2EEDataSourceTest and the comment is as following
          "If we are not in a transaction, we don't want to flow commit. We just return"
          6. For flowRollback: Like Kathey said, the original change is only for XAConnection which does not make sense. With the new change, server flow rollback if inUnitOfWork_ indicates we are not in a transaction, we return. However, with this change, StatementPoolingTest failed
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StatementPoolingTest.doTestResultSetCloseForHoldability(StatementPoolingTest.java:591)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StatementPoolingTest.resTestHoldCursorsOverCommit(StatementPoolingTest.java:522)

          I am in the middle of evaluating StatementPoolingTest failed and what change the value of inUnitOfWork_ when the test trying to close the connection and whether it has to with CachingLogicalConnection40. Will the test close the connection that is not the connection it performs rollback? What could be change the value of inUnitOfWork_ within StatementPoolingTest?

          Show
          Lily Wei added a comment - - edited Thanks Kathey for showing me around and making this patch to this stage and explain all the issues to me. With this patch, it has the intention fix for Connection.flowcommit() and Connection.flowrollback(). However, when running suites.All, some tests failed. i.e. jdbcapi.StatementPollingTest The change of the patch which answer the issue Kristian point out. Thanks Kristian for the looking at the patch and beyond. J2EEDataSourceTest 1. The test gets added to getClientSuite(). The test is only running on regular client, pooled, and xa datasources. 2. Change to javadoc as Kristian point out. 3. Please see comment in BaseJdbcTestCase.java 4. No need to use the system table as part of test anymore. BaseJdbcTestCase.java 5. getClientTransactonID(Connection conn) is in BaseJdbcTestCase.java so it is closer in suites than J2EEDataSourceTest and the comment is as following "If we are not in a transaction, we don't want to flow commit. We just return" 6. For flowRollback: Like Kathey said, the original change is only for XAConnection which does not make sense. With the new change, server flow rollback if inUnitOfWork_ indicates we are not in a transaction, we return. However, with this change, StatementPoolingTest failed at org.apache.derbyTesting.functionTests.tests.jdbcapi.StatementPoolingTest.doTestResultSetCloseForHoldability(StatementPoolingTest.java:591) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StatementPoolingTest.resTestHoldCursorsOverCommit(StatementPoolingTest.java:522) I am in the middle of evaluating StatementPoolingTest failed and what change the value of inUnitOfWork_ when the test trying to close the connection and whether it has to with CachingLogicalConnection40. Will the test close the connection that is not the connection it performs rollback? What could be change the value of inUnitOfWork_ within StatementPoolingTest?
          Hide
          Lily Wei added a comment - - edited

          I was looking at why StatementPoolingTest.test.doTestResultSetCloseorHoldability failure issue.
          When the first time it gets to this signature, conn.rollback() will flow. At that time CachingLoginalConnection40.physicalConnection_.inUnitofWork_ is false which is expected.
          However, when it gets to conn.close() which call StatementCacheInteractor.closeOpenLogicalStatements line 224 logicalStatement.close() the variable inUnitofWork_ become true. I am not familiar with closeOpenLogicalStatements synchronized method. Does this only get change in some sort of multi-thread situation?

          When StatementPoolingTest get to doTestResultSetCloseorHoldability() the second time, the variable of physicalConnection_.inUnitofWork_ remains false when close are completely done.

          What need to change so we can rely on inUnitofWork_ to be the flag to decide whether we can flow rollback or not?

          Show
          Lily Wei added a comment - - edited I was looking at why StatementPoolingTest.test.doTestResultSetCloseorHoldability failure issue. When the first time it gets to this signature, conn.rollback() will flow. At that time CachingLoginalConnection40.physicalConnection_.inUnitofWork_ is false which is expected. However, when it gets to conn.close() which call StatementCacheInteractor.closeOpenLogicalStatements line 224 logicalStatement.close() the variable inUnitofWork_ become true. I am not familiar with closeOpenLogicalStatements synchronized method. Does this only get change in some sort of multi-thread situation? When StatementPoolingTest get to doTestResultSetCloseorHoldability() the second time, the variable of physicalConnection_.inUnitofWork_ remains false when close are completely done. What need to change so we can rely on inUnitofWork_ to be the flag to decide whether we can flow rollback or not?
          Hide
          Lily Wei added a comment -

          From the debugger, we actually call Connection.completeTransactionStart(). This is the stack trace from conn.close->StatementCacheInterator.closeOpenLogicStatements ... Connection.completeTransactionStart()
          ===>
          NetConnection40(Connection).completeTransactionStart() line: 2082
          NetConnection40(Connection).readTransactionStart() line: 2078
          NetConnection40(NetConnection).readTransactionStart() line: 1581
          NetAgent(Agent).beginReadChain(Statement) line: 240
          NetAgent.beginReadChain(Statement) line: 488
          NetAgent(Agent).flow(Statement) line: 185
          PreparedStatement40(Statement).flowClose() line: 1682
          PreparedStatement40(Statement).resetForReuse() line: 334
          PreparedStatement40(PreparedStatement).resetForReuse() line: 127
          LogicalPreparedStatement40(LogicalStatementEntity).close() line: 175
          LogicalPreparedStatement40.close() line: 41
          StatementCacheInteractor.closeOpenLogicalStatements() line: 224

          Show
          Lily Wei added a comment - From the debugger, we actually call Connection.completeTransactionStart(). This is the stack trace from conn.close->StatementCacheInterator.closeOpenLogicStatements ... Connection.completeTransactionStart() ===> NetConnection40(Connection).completeTransactionStart() line: 2082 NetConnection40(Connection).readTransactionStart() line: 2078 NetConnection40(NetConnection).readTransactionStart() line: 1581 NetAgent(Agent).beginReadChain(Statement) line: 240 NetAgent.beginReadChain(Statement) line: 488 NetAgent(Agent).flow(Statement) line: 185 PreparedStatement40(Statement).flowClose() line: 1682 PreparedStatement40(Statement).resetForReuse() line: 334 PreparedStatement40(PreparedStatement).resetForReuse() line: 127 LogicalPreparedStatement40(LogicalStatementEntity).close() line: 175 LogicalPreparedStatement40.close() line: 41 StatementCacheInteractor.closeOpenLogicalStatements() line: 224
          Hide
          Lily Wei added a comment -

          Thanks to Kathey. I got to know more in turn of why Connection.close() after Connection.flowrollback() will cause error "Cannot close a connection while a transaction is still active" for StatementPoolingTest. As the ReproTransInProgressAttempt.java shows, if we have commit between we are in ResultSet, we should not flowrollback. If flowrollback remains unchanged, it will detect the state of the statement and resultSet and keep all the flag as inUnitOfWork etc in the right place and Connection.close() will not run into error as "Cannot close a connection while a transaction is still active".
          Due to this reason, I am leading toward to not fix flowrollback() to save round trip and just keep the change for flowcommit() to improve performance.

          Show
          Lily Wei added a comment - Thanks to Kathey. I got to know more in turn of why Connection.close() after Connection.flowrollback() will cause error "Cannot close a connection while a transaction is still active" for StatementPoolingTest. As the ReproTransInProgressAttempt.java shows, if we have commit between we are in ResultSet, we should not flowrollback. If flowrollback remains unchanged, it will detect the state of the statement and resultSet and keep all the flag as inUnitOfWork etc in the right place and Connection.close() will not run into error as "Cannot close a connection while a transaction is still active". Due to this reason, I am leading toward to not fix flowrollback() to save round trip and just keep the change for flowcommit() to improve performance.
          Hide
          Lily Wei added a comment -

          This patch is without Connection.flowrollback performance improvement fix.

          With the test, it is using the same approach as transaction id for Connection.flowcommit save round-trip fix. Move the getClientTransactionID to more test suite level - BaseJDBCTestCase.java

          It is ready to commit. Running Suites.All passed. I am running derbyall test suite now.

          Show
          Lily Wei added a comment - This patch is without Connection.flowrollback performance improvement fix. With the test, it is using the same approach as transaction id for Connection.flowcommit save round-trip fix. Move the getClientTransactionID to more test suite level - BaseJDBCTestCase.java It is ready to commit. Running Suites.All passed. I am running derbyall test suite now.
          Hide
          Lily Wei added a comment -

          Derbyall runs without an error either. The DERBY-4653-7_withflowcommittest.diff is ready for review or commit.

          Show
          Lily Wei added a comment - Derbyall runs without an error either. The DERBY-4653 -7_withflowcommittest.diff is ready for review or commit.
          Hide
          Kathey Marsden added a comment -

          Lily recommends putting in the change first for commit and has filed a second issue for rollback, That fix is perhaps not worth the change, because all open result sets would need to be checked before optimizing out the rollback flow.
          She filed another issue for rollback DERBY-4687
          , so I am changing the summary for this issue to cover just commit. I looked at the patch DERBY-4653-7_withflowcommittest.diff and I think it looks very good except a couple of stale comments. I will attach the revised patch and will commit Monday unless there are more comments.

          Show
          Kathey Marsden added a comment - Lily recommends putting in the change first for commit and has filed a second issue for rollback, That fix is perhaps not worth the change, because all open result sets would need to be checked before optimizing out the rollback flow. She filed another issue for rollback DERBY-4687 , so I am changing the summary for this issue to cover just commit. I looked at the patch DERBY-4653 -7_withflowcommittest.diff and I think it looks very good except a couple of stale comments. I will attach the revised patch and will commit Monday unless there are more comments.
          Hide
          Kathey Marsden added a comment -

          Here is the slightly revised patch. There was just a comment in the test that it ran with embedded, but it has been changed to run only with client.

          Show
          Kathey Marsden added a comment - Here is the slightly revised patch. There was just a comment in the test that it ran with embedded, but it has been changed to run only with client.
          Hide
          Kathey Marsden added a comment -

          Linking to separate issue for rollback. This one just covers commit now

          Show
          Kathey Marsden added a comment - Linking to separate issue for rollback. This one just covers commit now
          Hide
          Kristian Waagan added a comment -

          I attached an updated version of 'DERBY-4653-7_withflowcommittest_comment_update_diff.txt', with the following changes:

          • simplified the reflection code somewhat
          • JavaDoc edits
          • replaced tabs with spaces and some formatting changes (long lines)

          The patch looks okay, I have the following comments:
          a) I'm not thrilled by adding the LogicalConnection.getTransactionID method just for testing purposes (when we have another mechanism we could use), but I won't object.
          b) The argument 'expectednumtransaction' isn't used.
          c) The comment in the actual test is slightly inaccurate. Since the test case is run with autocommit on, the commit will be flowed at rs.close() such that none of the calls to commit() will cause a flow to the server.

          I'm +0.9 for commit

          I'll need to read up on the problems regarding rollback. I suppose most connection pools will issue a rollback rather than a commit to be sure the connection state is "clean", which is why it would be good to find an optimization for rollback as well.

          Show
          Kristian Waagan added a comment - I attached an updated version of ' DERBY-4653 -7_withflowcommittest_comment_update_diff.txt', with the following changes: simplified the reflection code somewhat JavaDoc edits replaced tabs with spaces and some formatting changes (long lines) The patch looks okay, I have the following comments: a) I'm not thrilled by adding the LogicalConnection.getTransactionID method just for testing purposes (when we have another mechanism we could use), but I won't object. b) The argument 'expectednumtransaction' isn't used. c) The comment in the actual test is slightly inaccurate. Since the test case is run with autocommit on, the commit will be flowed at rs.close() such that none of the calls to commit() will cause a flow to the server. I'm +0.9 for commit I'll need to read up on the problems regarding rollback. I suppose most connection pools will issue a rollback rather than a commit to be sure the connection state is "clean", which is why it would be good to find an optimization for rollback as well.
          Hide
          Kathey Marsden added a comment -

          I committed Kristian's version of DERBY-4653 and will leave to Lily to submit a follow up patch for the issues pointed out in his last comment. I think the trace file parsing test might be pretty fragile, so am not such a big fan.

          I agree that rollback is important. I will post a thought to issue DERBY-4687,

          Show
          Kathey Marsden added a comment - I committed Kristian's version of DERBY-4653 and will leave to Lily to submit a follow up patch for the issues pointed out in his last comment. I think the trace file parsing test might be pretty fragile, so am not such a big fan. I agree that rollback is important. I will post a thought to issue DERBY-4687 ,
          Hide
          Lily Wei added a comment -

          This patch take out the unused variable expectednumtransaction and set the test case to run with autocommit off so we will flow on first commit and does not flow the second commit.

          Show
          Lily Wei added a comment - This patch take out the unused variable expectednumtransaction and set the test case to run with autocommit off so we will flow on first commit and does not flow the second commit.
          Hide
          Kristian Waagan added a comment -

          Thanks Kathey and Lily.

          I committed the follow-up patch with revision 957164.
          Since Connection.setAutoCommit(boolean) may theoretically issue a commit, I moved it up before the call to fetch the transaction id.

          Show
          Kristian Waagan added a comment - Thanks Kathey and Lily. I committed the follow-up patch with revision 957164. Since Connection.setAutoCommit(boolean) may theoretically issue a commit, I moved it up before the call to fetch the transaction id.
          Hide
          Kristian Waagan added a comment -

          Attached patch 8a, which implements an alternative test parsing the client connection trace file.
          It's a bit more extensive than the existing test, but may not add any extra value.
          Uploaded for reference, as it may come in useful for debugging purposes.

          Show
          Kristian Waagan added a comment - Attached patch 8a, which implements an alternative test parsing the client connection trace file. It's a bit more extensive than the existing test, but may not add any extra value. Uploaded for reference, as it may come in useful for debugging purposes.
          Hide
          Kristian Waagan added a comment -

          Sorry, I missed the fact that DERBY-4709 was logged for the alternative test.
          Deleted the attachment and will upload it to the correct issue.

          Show
          Kristian Waagan added a comment - Sorry, I missed the fact that DERBY-4709 was logged for the alternative test. Deleted the attachment and will upload it to the correct issue.
          Hide
          Lily Wei added a comment -

          The code to save performance on Connection.flowcommit has been added. A short version test has also been added. Please also see DERBY-4709 and DERBY-4687

          Show
          Lily Wei added a comment - The code to save performance on Connection.flowcommit has been added. A short version test has also been added. Please also see DERBY-4709 and DERBY-4687
          Hide
          Kristian Waagan added a comment -

          Lily, can this issue be closed?

          Show
          Kristian Waagan added a comment - Lily, can this issue be closed?
          Hide
          Lily Wei added a comment -

          Thanks Kristian. I'll close this issue now.

          Show
          Lily Wei added a comment - Thanks Kristian. I'll close this issue now.

            People

            • Assignee:
              Lily Wei
              Reporter:
              Kristian Waagan
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development