Commons DbUtils
  1. Commons DbUtils
  2. DBUTILS-78

Add asynchronous batch, query, and update calls

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4
    • Labels:
      None

      Description

      I propose a new QueryRunner class, AsyncQueryRunner, which changes the return type of batch, query, and update methods. Instead of returning their respective return types, the methods would return a RunnableFuture. This would allow callers to either execute the RunnableFuture in a thread or via an CompletionService like the ExecutorCompletionService.

      I have attached a first cut at this class.

      1. DBUTILS-78_Future_v3.diff
        9 kB
        William R. Speirs
      2. DBUTILS-78_Future_v2.diff
        27 kB
        William R. Speirs
      3. DBUTILS-78_Future_v2.patch
        27 kB
        William R. Speirs
      4. DBUTILS-78_Future.patch
        23 kB
        Simone Tripodi
      5. 08_16_2011.diff
        3 kB
        William R. Speirs
      6. async.diff
        107 kB
        William R. Speirs
      7. pom.diff
        0.7 kB
        William R. Speirs
      8. AsyncQueryRunnerTest.java
        20 kB
        William R. Speirs
      9. AsyncQueryRunner.java
        29 kB
        William R. Speirs

        Activity

        Hide
        William R. Speirs added a comment -

        I wrote unit tests for the AsyncQueryRunner code. I am getting 73% coverage over the AsyncQueryRunner class. The only things that are not covered are a few exceptions in a few places; however, all methods are covered.

        I also replaced the version of my AsyncQueryRunner code after fixing numerous bugs I found while writing the unit tests.

        Show
        William R. Speirs added a comment - I wrote unit tests for the AsyncQueryRunner code. I am getting 73% coverage over the AsyncQueryRunner class. The only things that are not covered are a few exceptions in a few places; however, all methods are covered. I also replaced the version of my AsyncQueryRunner code after fixing numerous bugs I found while writing the unit tests.
        Hide
        Henri Yandell added a comment -

        Can you paste/attach your pom.xml modifications?

        Presumably you've updated junit and introduced a version of mockito?

        Show
        Henri Yandell added a comment - Can you paste/attach your pom.xml modifications? Presumably you've updated junit and introduced a version of mockito?
        Hide
        William R. Speirs added a comment -

        Any idea if/when this change will be integrated? Just looking for some feedback on the idea... Thanks!

        Show
        William R. Speirs added a comment - Any idea if/when this change will be integrated? Just looking for some feedback on the idea... Thanks!
        Hide
        Henri Yandell added a comment -

        Ack - I use iGoogle + RSS to notice when issues change. Apparently adding an attachment doesn't count. Sorry.

        Show
        Henri Yandell added a comment - Ack - I use iGoogle + RSS to notice when issues change. Apparently adding an attachment doesn't count. Sorry.
        Hide
        Henri Yandell added a comment -

        Patch applies; code compiles and tests pass.

        Two main issues I see are:

        1) Java 6 dependent (due to RunnableFuture). Currently DbUtils is 1.5. Given that 1.5 is EOL now, I don't think it's out of the question to move to Java 6.
        2) Need some class documentation providing an example of how to use it. Alternatively update the examples.xml; but I think moving examples back to the classes and package.html might be more valuable for DbUtils. The class documentation should also explain what it is (currently same as QueryRunner) and when you'd use it instead of QueryRunner.

        I recognize on the latter that this is asking more than QueryRunner has

        Show
        Henri Yandell added a comment - Patch applies; code compiles and tests pass. Two main issues I see are: 1) Java 6 dependent (due to RunnableFuture). Currently DbUtils is 1.5. Given that 1.5 is EOL now, I don't think it's out of the question to move to Java 6. 2) Need some class documentation providing an example of how to use it. Alternatively update the examples.xml; but I think moving examples back to the classes and package.html might be more valuable for DbUtils. The class documentation should also explain what it is (currently same as QueryRunner) and when you'd use it instead of QueryRunner. I recognize on the latter that this is asking more than QueryRunner has
        Hide
        William R. Speirs added a comment - - edited

        1) Yea, we'll need Java 6 for the future stuff. Wasn't sure what the project's posture on that is.
        2) Not a problem... documentation is key.

        However, I don't think this patch should be accepted as-is There is a LOT of duplicate code in my patch. I did this because I didn't want to mess with the QueryRunner code if everyone thought my idea was stupid. However, there are a number of methods in the AsyncQueryRunner are improved from the QueryRunner (see DBUTILS-79).

        I propose that we create a base or abstract class which both QueryRunner and AsyncQueryRunner extend. Thoughts?

        Show
        William R. Speirs added a comment - - edited 1) Yea, we'll need Java 6 for the future stuff. Wasn't sure what the project's posture on that is. 2) Not a problem... documentation is key. However, I don't think this patch should be accepted as-is There is a LOT of duplicate code in my patch. I did this because I didn't want to mess with the QueryRunner code if everyone thought my idea was stupid. However, there are a number of methods in the AsyncQueryRunner are improved from the QueryRunner (see DBUTILS-79 ). I propose that we create a base or abstract class which both QueryRunner and AsyncQueryRunner extend. Thoughts?
        Hide
        Henri Yandell added a comment -

        I've moved DbUtils to Java6. People can comment if they feel that is too soon.

        I've committed the patch - I agree that the classes should have a base class, but I think it'll be easier to do the other items (base class, documentation of example use and when you would use this) as subsequent patches.

        Show
        Henri Yandell added a comment - I've moved DbUtils to Java6. People can comment if they feel that is too soon. I've committed the patch - I agree that the classes should have a base class, but I think it'll be easier to do the other items (base class, documentation of example use and when you would use this) as subsequent patches.
        Hide
        Simone Tripodi added a comment -

        Hen,
        I agree on moving DBUtils to Java6: Java5 is no longer supported and in order to get as much as possible advantages - especially in the JDBC field - that move sounds more than reasonable.
        As a side note: in MyBatis (formerly Apache iBATIS) we had to take the same choice in order to take advantage from JDBC4 APIs.

        Show
        Simone Tripodi added a comment - Hen, I agree on moving DBUtils to Java6: Java5 is no longer supported and in order to get as much as possible advantages - especially in the JDBC field - that move sounds more than reasonable. As a side note: in MyBatis (formerly Apache iBATIS) we had to take the same choice in order to take advantage from JDBC4 APIs.
        Hide
        William R. Speirs added a comment -

        While I'm refactoring this, is it OK to remove the deprecated methods in QueryRunner?

        Show
        William R. Speirs added a comment - While I'm refactoring this, is it OK to remove the deprecated methods in QueryRunner?
        Hide
        William R. Speirs added a comment -

        I attached another patch which does the following:

        • Adds AbstractQueryRunner class
        • Removes common methods from QueryRunner and AsyncQueryRunner, both now extend AbstractQueryRunner
        • Updated the QueryRunner unit test to use Mockito; now with 74% code coverage (doesn't hit deprecated methods)
        Show
        William R. Speirs added a comment - I attached another patch which does the following: Adds AbstractQueryRunner class Removes common methods from QueryRunner and AsyncQueryRunner, both now extend AbstractQueryRunner Updated the QueryRunner unit test to use Mockito; now with 74% code coverage (doesn't hit deprecated methods)
        Hide
        William R. Speirs added a comment -

        Updated async.diff to include an update to the AsyncQueryRunner class and an example in the examples.xml file.

        Show
        William R. Speirs added a comment - Updated async.diff to include an update to the AsyncQueryRunner class and an example in the examples.xml file.
        Hide
        Henri Yandell added a comment -

        Adding abstract looks good.

        Presumably "public int update(Connection conn, boolean closeConn, String sql, Object... params)" was meant to be private?

        I think the example should assume the use case that there are two separate pieces of code. That is, the take().get() call should be in a separate block. That will also make the exception handling clearer for the second block.

        The new parameter protection code looks ok (DBUTILS-79). There's an odd bit where you call close(conn) after identifying conn was null.

        I've applied all the changes (r1158107, r1158109, r1158110). Let me know your thoughts on the above.

        Show
        Henri Yandell added a comment - Adding abstract looks good. Presumably "public int update(Connection conn, boolean closeConn, String sql, Object... params)" was meant to be private? I think the example should assume the use case that there are two separate pieces of code. That is, the take().get() call should be in a separate block. That will also make the exception handling clearer for the second block. The new parameter protection code looks ok ( DBUTILS-79 ). There's an odd bit where you call close(conn) after identifying conn was null. I've applied all the changes (r1158107, r1158109, r1158110). Let me know your thoughts on the above.
        Hide
        William R. Speirs added a comment -

        Henri, good eyes! The public -> private and closing the null connections were all mistakes. I also updated the example to break it up into two try/catch blocks. If you think it needs further changes, go for it. These changes are attached as 08_16_2011.diff.

        The only other thing would be dropping the deprecated methods from the QueryRunner class. Thoughts on that?

        Thanks...

        Show
        William R. Speirs added a comment - Henri, good eyes! The public -> private and closing the null connections were all mistakes. I also updated the example to break it up into two try/catch blocks. If you think it needs further changes, go for it. These changes are attached as 08_16_2011.diff. The only other thing would be dropping the deprecated methods from the QueryRunner class. Thoughts on that? Thanks...
        Hide
        Henri Yandell added a comment -

        Patch applied. I changed the exception on the 2nd example block to be an InterruptedException.

        On the deprecation, it depends on what version we release as. If we stay as 1.4, then we would probably hang on to them. If we move to 2.0 then they would go.

        You changed the code to only depend on Callable, so I think it's JDK 1.5 dependent again which decreases the need to do a 2.0.

        Show
        Henri Yandell added a comment - Patch applied. I changed the exception on the 2nd example block to be an InterruptedException. On the deprecation, it depends on what version we release as. If we stay as 1.4, then we would probably hang on to them. If we move to 2.0 then they would go. You changed the code to only depend on Callable, so I think it's JDK 1.5 dependent again which decreases the need to do a 2.0.
        Hide
        Henri Yandell added a comment -

        Resolving the issue - many thanks for all the hard work William

        I'm going to roll the pom.xml back to being JDK 1.5 dependent for now.

        Show
        Henri Yandell added a comment - Resolving the issue - many thanks for all the hard work William I'm going to roll the pom.xml back to being JDK 1.5 dependent for now.
        Hide
        Simone Tripodi added a comment -

        I think we could do a little step forward to avoid users have to write redundant code, adding in the constructors the ExecutorService to run async code and changing the methods signatures returning Future instead of Callable.
        I'm going to attach a patch with a PoC so we can discuss if including it or not in 1.5

        Show
        Simone Tripodi added a comment - I think we could do a little step forward to avoid users have to write redundant code, adding in the constructors the ExecutorService to run async code and changing the methods signatures returning Future instead of Callable . I'm going to attach a patch with a PoC so we can discuss if including it or not in 1.5
        Hide
        Simone Tripodi added a comment -

        Please take the attached patch (DBUTILS-78_Future.patch) as a pure PoC because there are test failures.
        But if the modifications would make sense, I can work on having it fully working and committing it - or feel free to submit a patch yourself and than I can apply it, just let me know.

        Show
        Simone Tripodi added a comment - Please take the attached patch ( DBUTILS-78 _Future.patch) as a pure PoC because there are test failures. But if the modifications would make sense, I can work on having it fully working and committing it - or feel free to submit a patch yourself and than I can apply it, just let me know.
        Hide
        William R. Speirs added a comment -

        OK, I will take a look tomorrow.

        Bill-
        https://issues.apache.org/jira/browse/DBUTILS-78?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel]
        because there are test failures.
        working and committing it - or feel free to submit a patch yourself and than
        I can apply it, just let me know.
        AsyncQueryRunnerTest.java, DBUTILS-78_Future.patch, async.diff, pom.diff
        return type of batch, query, and update methods. Instead of returning their
        respective return types, the methods would return a RunnableFuture. This
        would allow callers to either execute the RunnableFuture in a thread or via
        an CompletionService like the ExecutorCompletionService.

        Show
        William R. Speirs added a comment - OK, I will take a look tomorrow. Bill- https://issues.apache.org/jira/browse/DBUTILS-78?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] because there are test failures. working and committing it - or feel free to submit a patch yourself and than I can apply it, just let me know. AsyncQueryRunnerTest.java, DBUTILS-78 _Future.patch, async.diff, pom.diff return type of batch, query, and update methods. Instead of returning their respective return types, the methods would return a RunnableFuture. This would allow callers to either execute the RunnableFuture in a thread or via an CompletionService like the ExecutorCompletionService.
        Hide
        William R. Speirs added a comment -

        Overall looks good! A few things:

        1) I cannot think of a case where the Callable which is returned wouldn't simply be placed on some ExecutorService, but just because I cannot think of a case doesn't mean someone might not want to do it. If we go with this patch we force people's hand to use an ExecutorService. Again, I don't think it's a big deal, but I'd to get other people's thoughts.

        2) The tests which are failing are all failing because of a check to see if the statement hasn't been closed after calling batch, update, or query before calling get(). However, now that we're using an ExecutorService this "get()" call is being executed for us, so the check is no longer valid. I've removed the check from the test case so that all of the tests now pass. (Note: we still check to make sure the statements are all properly closed at the end.)

        I attached a new patch which includes these changes: DBUTILS-78_Future_v2.patch

        Bill-

        Show
        William R. Speirs added a comment - Overall looks good! A few things: 1) I cannot think of a case where the Callable which is returned wouldn't simply be placed on some ExecutorService, but just because I cannot think of a case doesn't mean someone might not want to do it. If we go with this patch we force people's hand to use an ExecutorService. Again, I don't think it's a big deal, but I'd to get other people's thoughts. 2) The tests which are failing are all failing because of a check to see if the statement hasn't been closed after calling batch, update, or query before calling get(). However, now that we're using an ExecutorService this "get()" call is being executed for us, so the check is no longer valid. I've removed the check from the test case so that all of the tests now pass. (Note: we still check to make sure the statements are all properly closed at the end.) I attached a new patch which includes these changes: DBUTILS-78 _Future_v2.patch Bill-
        Hide
        Simone Tripodi added a comment -

        Thanks a lot for your prompt help William, very appreciated!

        I proposed to use ExecutorService due to my not big experience with concurrent APIs, and just reading basic javadoc I found that ExecutorService is the more primitive element to run {{Callable}}s.
        So apologize for having reopened the issue, I thought that just providing ExecutorService would reduce code redundancies.

        Anyway, since you submitted the initial patch (I guess you had a usecase for having submitted it), if you can confirm that the new version of AsyncQueryRunner would be really useful, I would be pleased to apply it.

        Many thanks in advance!
        Simo

        Show
        Simone Tripodi added a comment - Thanks a lot for your prompt help William, very appreciated! I proposed to use ExecutorService due to my not big experience with concurrent APIs, and just reading basic javadoc I found that ExecutorService is the more primitive element to run {{Callable}}s. So apologize for having reopened the issue, I thought that just providing ExecutorService would reduce code redundancies. Anyway, since you submitted the initial patch (I guess you had a usecase for having submitted it), if you can confirm that the new version of AsyncQueryRunner would be really useful, I would be pleased to apply it. Many thanks in advance! Simo
        Hide
        William R. Speirs added a comment -

        My use case simply takes the result of my patch, Callable, and submits it to an ExecutorService. It never occurred to me to simply provide the API with the ExecutorService and let it do the submission.

        Anyone else reading this have thoughts on this? Otherwise, I like the patch!

        Show
        William R. Speirs added a comment - My use case simply takes the result of my patch, Callable, and submits it to an ExecutorService. It never occurred to me to simply provide the API with the ExecutorService and let it do the submission. Anyone else reading this have thoughts on this? Otherwise, I like the patch!
        Hide
        Simone Tripodi added a comment -

        William,
        I am experiencing issues when applying the patch:

        commons-dbutils simonetripodi$ patch -p0 < DBUTILS-78_Future_v2.patch 
        (Stripping trailing CRs from patch.)
        can't find file to patch at input line 5
        Perhaps you used the wrong -p or --strip option?
        The text leading up to this was:
        --------------------------
        |Index: main/java/org/apache/commons/dbutils/AsyncQueryRunner.java
        |===================================================================
        |--- main/java/org/apache/commons/dbutils/AsyncQueryRunner.java	(revision 1175857)
        |+++ main/java/org/apache/commons/dbutils/AsyncQueryRunner.java	(working copy)
        --------------------------
        File to patch: 
        

        Can you help me please? TIA!

        Show
        Simone Tripodi added a comment - William, I am experiencing issues when applying the patch: commons-dbutils simonetripodi$ patch -p0 < DBUTILS-78_Future_v2.patch (Stripping trailing CRs from patch.) can't find file to patch at input line 5 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |Index: main/java/org/apache/commons/dbutils/AsyncQueryRunner.java |=================================================================== |--- main/java/org/apache/commons/dbutils/AsyncQueryRunner.java (revision 1175857) |+++ main/java/org/apache/commons/dbutils/AsyncQueryRunner.java (working copy) -------------------------- File to patch: Can you help me please? TIA!
        Hide
        William R. Speirs added a comment -

        Hmmm... I'm on a Windows box and used TortoiseSVN and created the patch as I normally do. It is a patch "on top" of your patch... essentially I applied your patch, made changes, and then created another patch. I can try it again and re-attach, but I've created patches this way before and they've worked so I'm not really sure...

        Show
        William R. Speirs added a comment - Hmmm... I'm on a Windows box and used TortoiseSVN and created the patch as I normally do. It is a patch "on top" of your patch... essentially I applied your patch, made changes, and then created another patch. I can try it again and re-attach, but I've created patches this way before and they've worked so I'm not really sure...
        Hide
        William R. Speirs added a comment -

        Try DBUTILS-78_Future_v2.diff

        Show
        William R. Speirs added a comment - Try DBUTILS-78 _Future_v2.diff
        Hide
        Simone Tripodi added a comment -

        Thanks William for your help, I reintegrated the latest patch to r1176052 - unfortunately I wasn't able to manage it via patch command, so I reapplied mine first and then had a look at your modifications.
        Thanks anyway for the new feature!

        Show
        Simone Tripodi added a comment - Thanks William for your help, I reintegrated the latest patch to r1176052 - unfortunately I wasn't able to manage it via patch command, so I reapplied mine first and then had a look at your modifications. Thanks anyway for the new feature!
        Hide
        Simone Tripodi added a comment -

        Patch applied, thanks for contributing William!!!

        Show
        Simone Tripodi added a comment - Patch applied, thanks for contributing William!!!

          People

          • Assignee:
            Simone Tripodi
            Reporter:
            William R. Speirs
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development