OpenJPA
  1. OpenJPA
  2. OPENJPA-1067

SetQueryTimeout(x) where x != 0 causes SQLException with DB2 on Z/OS

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.0.3, 1.2.1, 1.3.0, 2.0.0-M2
    • Fix Version/s: 1.0.4, 1.2.2, 1.3.0, 2.0.0-M2
    • Component/s: None
    • Labels:
      None

      Description

      A recent (AFAIK) update to the DB2 JDBC driver changed behavior when the setQueryTimeout method is called on a connection to Z/OS and the timeout was non 0.

      A non zero value is not supported, but previously the value was ignored. Now an SQLException is raised.

        Issue Links

          Activity

          Hide
          Donald Woods added a comment -

          Interesting... do you want me to look at this, given I modified how setQueryTimeout() is called in trunk to support the new javax.persistence.query.timeout hint?

          Show
          Donald Woods added a comment - Interesting... do you want me to look at this, given I modified how setQueryTimeout() is called in trunk to support the new javax.persistence.query.timeout hint?
          Hide
          Michael Dick added a comment -

          Hi Donald, I appreciate the offer. My proposed solution is to set supportsQueryTimeout = false for Z/OS in the DB2Dictionary. Preliminary testing works with 1.2.1.

          I don't think it pays to invent something new that handles the value of 0. If that's the only value it accepts presumably that's the default anyway.

          Show
          Michael Dick added a comment - Hi Donald, I appreciate the offer. My proposed solution is to set supportsQueryTimeout = false for Z/OS in the DB2Dictionary. Preliminary testing works with 1.2.1. I don't think it pays to invent something new that handles the value of 0. If that's the only value it accepts presumably that's the default anyway.
          Hide
          Donald Woods added a comment -

          OK, but should we disable query timeout support for all DB2 zOS users, or just catch and log any exceptions when calling setQueryTimeout(), which could be useful for other drivers too. By disabling query timeout support, you also disable the current lock timeout support which relies on using setQueryTimeout().

          Show
          Donald Woods added a comment - OK, but should we disable query timeout support for all DB2 zOS users, or just catch and log any exceptions when calling setQueryTimeout(), which could be useful for other drivers too. By disabling query timeout support, you also disable the current lock timeout support which relies on using setQueryTimeout().
          Hide
          Michael Dick added a comment -

          It's a bit of a tradeoff. We know this call will at best have no effect, and at worst throws an exception. Either way we've wasted a trip to the database.

          I think in a perfect world it'd be better to have the functions that rely on query timeouts check whether the DBDictionary supports them and provide feedback (exception or log) when they're called. That might be more trouble than it's worth though - I haven't looked at the affected code.

          Before I saw your comment I went ahead and disabled it. I don't think there's any harm in leaving that code in and leaving the JIRA open though.

          Show
          Michael Dick added a comment - It's a bit of a tradeoff. We know this call will at best have no effect, and at worst throws an exception. Either way we've wasted a trip to the database. I think in a perfect world it'd be better to have the functions that rely on query timeouts check whether the DBDictionary supports them and provide feedback (exception or log) when they're called. That might be more trouble than it's worth though - I haven't looked at the affected code. Before I saw your comment I went ahead and disabled it. I don't think there's any harm in leaving that code in and leaving the JIRA open though.
          Hide
          Michael Dick added a comment -

          Actually on second hand I think there's a separate issue here : whether we should remove the supportsQueryTimeout field.

          Selectively disabling some methods like we are with supportsQueryTimeout is problematic and it's easy to get out of sync with JDBC vendors (like we did in this case). We could let the call go through and let the upstream callers handle the exception - rather than checking for supportsQueryTimeout before calling the method.

          That's not a change I'm comfortable making in 1.2.x or 1.0.x though. Negating the call and logging could be done though.

          There's a fair amount of precedent for checking before calling though (avoiding the trip to the database). Unless there are objections I'll open a sub-task / related issue to discuss whether we need the supportsQueryTimeout field.

          Show
          Michael Dick added a comment - Actually on second hand I think there's a separate issue here : whether we should remove the supportsQueryTimeout field. Selectively disabling some methods like we are with supportsQueryTimeout is problematic and it's easy to get out of sync with JDBC vendors (like we did in this case). We could let the call go through and let the upstream callers handle the exception - rather than checking for supportsQueryTimeout before calling the method. That's not a change I'm comfortable making in 1.2.x or 1.0.x though. Negating the call and logging could be done though. There's a fair amount of precedent for checking before calling though (avoiding the trip to the database). Unless there are objections I'll open a sub-task / related issue to discuss whether we need the supportsQueryTimeout field.
          Hide
          Donald Woods added a comment -

          We have existing code and tests that check for (supportsQueryTimeout == true) today (like the reworked setTimeouts and setQueryTimeout in DBDictionary in trunk) so there would be some minor rework. Adding a try/catch/log wrapper to any setQueryTiemout() calls along with always calling getQueryTimeout() before setting a value, would probably be the better route for 1.3/2.0. I'd expect DB2 to fix this in the next driver release, given this is a regression/loss of function for existing users...

          Show
          Donald Woods added a comment - We have existing code and tests that check for (supportsQueryTimeout == true) today (like the reworked setTimeouts and setQueryTimeout in DBDictionary in trunk) so there would be some minor rework. Adding a try/catch/log wrapper to any setQueryTiemout() calls along with always calling getQueryTimeout() before setting a value, would probably be the better route for 1.3/2.0. I'd expect DB2 to fix this in the next driver release, given this is a regression/loss of function for existing users...
          Hide
          Michael Dick added a comment -

          As far as I can tell it never worked for Z/OS - this has been a restriction as far back as I could check.

          I'm guessing their response would be that adding support would be a new feature request (could take some time), and would we rather have them throw the exception or just ignore it. Which is pretty much what we're talking about

          I've started talking to the DB2 contacts I know, and I'll update the issue when I know more.

          Show
          Michael Dick added a comment - As far as I can tell it never worked for Z/OS - this has been a restriction as far back as I could check. I'm guessing their response would be that adding support would be a new feature request (could take some time), and would we rather have them throw the exception or just ignore it. Which is pretty much what we're talking about I've started talking to the DB2 contacts I know, and I'll update the issue when I know more.
          Hide
          Michael Dick added a comment -

          For released versions (1.0.x - 1.2.x) I'm leaning towards something like the attached patch (for 1.2.x).

          In trunk we may want to do something better (ie separate dictionaries for DB2ZOS to make this a little less kludgy, and as Donald said the code path is a little different so it might need some tweaking.

          The patch uses jMock in the testcase to simulate a connection to Z so if you just apply it in an IDE you may need to regen your list of dependencies (mvn eclipse:eclipse, or something similar).

          Show
          Michael Dick added a comment - For released versions (1.0.x - 1.2.x) I'm leaning towards something like the attached patch (for 1.2.x). In trunk we may want to do something better (ie separate dictionaries for DB2ZOS to make this a little less kludgy, and as Donald said the code path is a little different so it might need some tweaking. The patch uses jMock in the testcase to simulate a connection to Z so if you just apply it in an IDE you may need to regen your list of dependencies (mvn eclipse:eclipse, or something similar).

            People

            • Assignee:
              Michael Dick
              Reporter:
              Michael Dick
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development