Commons DbUtils
  1. Commons DbUtils
  2. DBUTILS-88

Make AsyncQueryRunner be a decorator around a QueryRunner

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.5
    • Labels:
      None

      Description

      AsyncQueryRunner duplicates much of the code in QueryRunner. Would it be possible for AsyncQueryRunner to simply decorate a QueryRunner with async functionality, in the same way a BufferedInputStream might decorate an InputStream?

      1. DBUTILS-88v2.patch
        18 kB
        Moandji Ezana
      2. DBUTILS-88v1.patch
        47 kB
        William R. Speirs
      3. AsyncQueryRunner_wraps_QueryRunner.txt
        12 kB
        Moandji Ezana

        Activity

        Hide
        Moandji Ezana added a comment -

        AsyncQueryRunner_wraps_QueryRunner.txt is a patch that demonstrates my idea. It does not update the tests, which currently fail.

        Show
        Moandji Ezana added a comment - AsyncQueryRunner_wraps_QueryRunner.txt is a patch that demonstrates my idea. It does not update the tests, which currently fail.
        Hide
        William R. Speirs added a comment -

        I took this patch one step further and removed the private methods in AsyncQueryRunner as they're not really needed. This also allowed me to remove extending AbstractQueryRunner.

        However, this has API issues. Unfortunately, the inner classes in AsyncQueryRunner were marked as protected not private, so removing them changes the API. The same is true with removing extending AbstractQueryRunner.

        For 1.5 we can always leave these classes and simply mark them as @Deprecated and then finally remove the extension of AbstractQueryRunner in v2.0.

        Thoughts?

        Show
        William R. Speirs added a comment - I took this patch one step further and removed the private methods in AsyncQueryRunner as they're not really needed. This also allowed me to remove extending AbstractQueryRunner. However, this has API issues. Unfortunately, the inner classes in AsyncQueryRunner were marked as protected not private, so removing them changes the API. The same is true with removing extending AbstractQueryRunner. For 1.5 we can always leave these classes and simply mark them as @Deprecated and then finally remove the extension of AbstractQueryRunner in v2.0. Thoughts?
        Hide
        William R. Speirs added a comment - - edited

        Note: The DBUTILS-88v1.patch includes changes to the unit tests which all pass now.

        Show
        William R. Speirs added a comment - - edited Note: The DBUTILS-88 v1.patch includes changes to the unit tests which all pass now.
        Hide
        Moandji Ezana added a comment -

        Looks good to me. It might be worth marking the other constructors as @Deprecated, too. If AsyncQueryRunner is about wrapping a QueryRunner, AsyncQueryRunner(ExecutorService executorService, QueryRunner queryRunner) best expresses that.

        Ultimately, it might be worth making QueryRunner an interface that is implemented by both AsyncQueryRunner and the current QueryRunner (renamed to BasicQueryRunner?).

        Show
        Moandji Ezana added a comment - Looks good to me. It might be worth marking the other constructors as @Deprecated, too. If AsyncQueryRunner is about wrapping a QueryRunner, AsyncQueryRunner(ExecutorService executorService, QueryRunner queryRunner) best expresses that. Ultimately, it might be worth making QueryRunner an interface that is implemented by both AsyncQueryRunner and the current QueryRunner (renamed to BasicQueryRunner?).
        Hide
        Moandji Ezana added a comment -

        DBUTILS-88v2.patch restores API compatibility by making AsyncQueryRunner extend AbstractQueryRunner again and undeletes the inner classes. It also adds deprecation warnings.

        Show
        Moandji Ezana added a comment - DBUTILS-88 v2.patch restores API compatibility by making AsyncQueryRunner extend AbstractQueryRunner again and undeletes the inner classes. It also adds deprecation warnings.
        Hide
        Moandji Ezana added a comment - - edited

        Does anything need to happen for this issue and DBUTILS-87 to be committed?

        Show
        Moandji Ezana added a comment - - edited Does anything need to happen for this issue and DBUTILS-87 to be committed?
        Hide
        William R. Speirs added a comment -

        I'll take another look and commit the patch. However, I've had trouble in the past and I'm unable to release a new version of DBUtils. I'll see if I can get some help on that and get this done by the end of the week.

        Show
        William R. Speirs added a comment - I'll take another look and commit the patch. However, I've had trouble in the past and I'm unable to release a new version of DBUtils. I'll see if I can get some help on that and get this done by the end of the week.
        Hide
        William R. Speirs added a comment -

        Everything looked good so I committed your patch. I will try and get someone to release this version of DBUtils so we can start getting feedback from the community.

        Show
        William R. Speirs added a comment - Everything looked good so I committed your patch. I will try and get someone to release this version of DBUtils so we can start getting feedback from the community.
        Hide
        William R. Speirs added a comment -

        Patch applied in r1305706

        Show
        William R. Speirs added a comment - Patch applied in r1305706
        Hide
        Simone Tripodi added a comment -

        1.5-RC released

        Show
        Simone Tripodi added a comment - 1.5-RC released

          People

          • Assignee:
            Unassigned
            Reporter:
            Moandji Ezana
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development