Uploaded image for project: 'OpenJPA'
  1. OpenJPA
  2. OPENJPA-2517

Incorrect the time unit of query timeout value.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.0, 2.2.1, 2.2.2, 2.3.0
    • Fix Version/s: 2.2.3, 2.4.1
    • Component/s: jdbc
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      The value of the "javax.persistence.query.timeout" property have been passed to the java.sql.Statement.setQueryTimeout(int) in milliseconds rather than seconds.

      The query timeout milliseconds should be converted to seconds.

      1. openjpa-querytimeout-bug.zip
        6 kB
        Masafumi Koba
      2. OPENJPA-2517.patch
        0.9 kB
        Masafumi Koba
      3. openjpa-querytimeout-working.zip
        6 kB
        Heath Thomann
      4. OPENJPA-2517-2.2.x.patch
        4 kB
        Heath Thomann

        Activity

        Hide
        jpaheath Heath Thomann added a comment -

        HI! I've recently taken another look at this issue. I wanted to try to summarize all the comments made so far, and add some clarification. Basically, there are three issues with OpenJPA's handling of javax.persistence.query.timeout (QT for short):

        1) OpenJPA applies the QT to all EM operations (e.g find/update).
        2) OpenJPA uses seconds as the QT rather than milliseconds in #1.
        3) OpenJPA doesn't round up QT values less than 1000.

        It is explained in previous comments that, as per the JPA Spec, the QT should only apply to Query operations (methods on the Query interface). Let me be clear and state that OpenJPA does properly apply the QT to Query operations. There are no issues for Query operations. The issues here are with the QT being applied to EM operations (#1). Given the fact that the QT is applied to EM operations when it shouldn't, #2 and #3 are moot. However, since it was decided not to remove the QT from EM operations because of the risk of regression, #2 and #3 are not consistent with the way the QT is handled for Query operations. For Query operations, OpenJPA rounds the QT to 1000 if the user sets the QT to a value less than 1000. Furthermore, for Query operations OpenJPA converts the milliseconds to seconds and passes that value to the JDBC driver since the driver expects units of seconds.

        The purpose of the fix provider in this JIRA, and the property openjpa.jdbc.DBDictionary.allowQueryTimeoutOnFindUpdate, was to continue to allow #1, but resolve #2 and #3. In other words, we did not want to run the risk of regressing users by change #1 such that the QT no longer applies to all EM operations. At a later date, and another JIRA, we could investigate a change to provide a property to fix #1 by restricting the QT to only Query operations. The fix of this JIRA however fixes #2 and #3 in order to maintain a consistent story between EM operations and Query operations. In other words, when property openjpa.jdbc.DBDictionary.allowQueryTimeoutOnFindUpdate is set to true, the QT will be converted to milliseconds, and any value less than 1000 will be rounded up to 1000.

        Finally, I would like to note that a user can set the QT to 0 to entirely disable the setting of a timeout. This would disabled the QT for both Query and EM operations.

        Thanks,

        Heath

        Show
        jpaheath Heath Thomann added a comment - HI! I've recently taken another look at this issue. I wanted to try to summarize all the comments made so far, and add some clarification. Basically, there are three issues with OpenJPA's handling of javax.persistence.query.timeout (QT for short): 1) OpenJPA applies the QT to all EM operations (e.g find/update). 2) OpenJPA uses seconds as the QT rather than milliseconds in #1. 3) OpenJPA doesn't round up QT values less than 1000. It is explained in previous comments that, as per the JPA Spec, the QT should only apply to Query operations (methods on the Query interface). Let me be clear and state that OpenJPA does properly apply the QT to Query operations. There are no issues for Query operations. The issues here are with the QT being applied to EM operations (#1). Given the fact that the QT is applied to EM operations when it shouldn't, #2 and #3 are moot. However, since it was decided not to remove the QT from EM operations because of the risk of regression, #2 and #3 are not consistent with the way the QT is handled for Query operations. For Query operations, OpenJPA rounds the QT to 1000 if the user sets the QT to a value less than 1000. Furthermore, for Query operations OpenJPA converts the milliseconds to seconds and passes that value to the JDBC driver since the driver expects units of seconds. The purpose of the fix provider in this JIRA, and the property openjpa.jdbc.DBDictionary.allowQueryTimeoutOnFindUpdate, was to continue to allow #1, but resolve #2 and #3. In other words, we did not want to run the risk of regressing users by change #1 such that the QT no longer applies to all EM operations. At a later date, and another JIRA, we could investigate a change to provide a property to fix #1 by restricting the QT to only Query operations. The fix of this JIRA however fixes #2 and #3 in order to maintain a consistent story between EM operations and Query operations. In other words, when property openjpa.jdbc.DBDictionary.allowQueryTimeoutOnFindUpdate is set to true, the QT will be converted to milliseconds, and any value less than 1000 will be rounded up to 1000. Finally, I would like to note that a user can set the QT to 0 to entirely disable the setting of a timeout. This would disabled the QT for both Query and EM operations. Thanks, Heath
        Hide
        struberg Mark Struberg added a comment -

        Heath, thanks for porting this over to trunk as well. Thus I've added 2.4.1 to the 'fixed in' versions. Could you please add the other branches where you applied this fix as well so we can properly keep track of it in our release notes? txs!

        Show
        struberg Mark Struberg added a comment - Heath, thanks for porting this over to trunk as well. Thus I've added 2.4.1 to the 'fixed in' versions. Could you please add the other branches where you applied this fix as well so we can properly keep track of it in our release notes? txs!
        Hide
        mskb Masafumi Koba added a comment -

        Hi! I confirmed the commit. No problem. Thank you!

        Show
        mskb Masafumi Koba added a comment - Hi! I confirmed the commit. No problem. Thank you!
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1700885 from Heath Thomann in branch 'openjpa/trunk'
        [ https://svn.apache.org/r1700885 ]

        OPENJPA-2517: Option to allow the javax.persistence.query.timeout property to apply to EntityManager operations. Ported 2.2.x changes to trunk.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1700885 from Heath Thomann in branch 'openjpa/trunk' [ https://svn.apache.org/r1700885 ] OPENJPA-2517 : Option to allow the javax.persistence.query.timeout property to apply to EntityManager operations. Ported 2.2.x changes to trunk.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1700884 from Heath Thomann in branch 'openjpa/branches/2.2.x'
        [ https://svn.apache.org/r1700884 ]

        OPENJPA-2517: Option to allow the javax.persistence.query.timeout property to apply to EntityManager operations.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1700884 from Heath Thomann in branch 'openjpa/branches/2.2.x' [ https://svn.apache.org/r1700884 ] OPENJPA-2517 : Option to allow the javax.persistence.query.timeout property to apply to EntityManager operations.
        Hide
        jpaheath Heath Thomann added a comment -

        HI! I'm attaching a test fix which will allow a user to set a property (I've chosen openjpa.jdbc.DBDictionary.allowQueryTimeoutOnFindUpdate) which when set will apply the javax.persistence.query.timeout, in milliseconds, to EntityManager operations. As I explained in my previous post, today the query.timeout is applied to EM operations in seconds. Please review and provide feedback.

        Thanks,

        Heath Thomann

        Show
        jpaheath Heath Thomann added a comment - HI! I'm attaching a test fix which will allow a user to set a property (I've chosen openjpa.jdbc.DBDictionary.allowQueryTimeoutOnFindUpdate) which when set will apply the javax.persistence.query.timeout, in milliseconds, to EntityManager operations. As I explained in my previous post, today the query.timeout is applied to EM operations in seconds. Please review and provide feedback. Thanks, Heath Thomann
        Hide
        jpaheath Heath Thomann added a comment -

        OK, I have talked to Rick Curtis and Kevin Sutter about this some more and I have to add some more details and slightly back track. This post is going to be extremely long, however, I want to add lots of details for history sake. In this post I'm also going to do something that is probably very unorthodox, and that is I'm going to explain the way EclipseLink (ECL) handles the 'javax.persistence.query.timeout' (QT for short). I know, I know, I shouldn't mix the two, however it appears that two major JPA providers are incorrectly handling the QT so I think it is worth pointing this out as it may also help to justify our final proposed handling of QT.

        I'd suggest the reader read the email I sent to Rick and Kevin for back ground [1]. This email details what OpenJPA is doing wrong when it comes to handling of the QT during a find/update scenario. Basically OpenJPA does two things wrong:

        1) It applies the "javax.persistence.query.timeout" value to a find/update (an EM operation).
        2) It uses units of seconds, rather than milliseconds, in the find/update case.

        As I explained back on May 27th, the spec is slightly vague about the application of QT. But after a careful read, you can see that the QT should NOT apply to EM operations, only Query operations. Therefore, #1 by OpenJPA (and ECL) is incorrect. However, given that OpenJPA has applied the QT to EM operations since the inception of QT, we feel it is best to continue to do so. We do not what to fix this and break all users who may directly or indirectly rely on this. At some point we could add options to enable/disable this (although OpenJPA does allow a user to disable the QT.....actually this is the default value). Also keep in mind that the JPA 2.0 and 2.1 specs both point out that the QT is just a hint, and a user should not completely depend on it, so I think there is further wiggle room for interpretation and implementation. Furthermore, customer's have argued that the QT should apply to all operations; both EM and Query. IMHO I agree with them.....it seems odd that one can control a timeout for a query, but NOT for an exactly same find/update operation.
        Next, given that we are not going to change things, we do feel we should address #2. We do feel we should do as the originator of this JIRA suggested, that is, to properly convert the QT value in 'ConfiguringConnectionDecorator' to milliseconds, from seconds. The DBDictionary converts the QT from milliseconds to seconds, the 'ConfiguringConnectionDecorator' should follow suite for consistency.

        [1] Email to Kevin Sutter, Rick Curtis, et al.:
        >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>EMAIL START>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
        Hey Team! I need help with a likely bug in OpenJPA (and ECL?) with our javax.persistence.query.timeout (QT for short) usage. I know some of you worked on a QT issue on ECL.....my issue might also apply to ECL. This issue takes a bit of time to describe so settle in with a cup of coffee or tea. Basically though the root question is: should the QT apply to a finder+update (EM operation, no Query)? If so both OJ and ECL have a bug.
        To start, you could read my details in OJ2517, and look at the test. But even these details aren't exactly the same as my latest issue. In the JIRA the user was expecting the QT to apply to a EM.find/update scenario. In other words, they expected the QT to apply to ALL EM operations. However, as I explained in the JIRA the QT should only apply to javax.persistence.Query operations......Kevin's email [removed] seemed to back this up, the spec 'implies' this, and the TestQueryTimout test written by Pinaki would seem to offer further proof. However, I've found a scenario where both OJ and ECL apply the QT to an EM.find/update. Let me explain exactly what I mean by a 'EM.find/update' by using a code snippet to describe the scenario:

        Thread 1 (T1) uses JDBC to 'lock' a row with id 1, in a table name Bug, and then sleeps, as follows:

        int jdbcDelayTime = 9000;
        Connection conn = .......
        Statement stmt = conn.createStatement();
        stmt.execute("UPDATE Bug set title = 'a' where bugId = 1");
        System.out.println("\nJDBC Thread will now sleep for " + jdbcDelayTime + " ms");
        // emulate long process to hold lock on the row:
        Thread.sleep(jdbcDelayTime);

        Thread 2 (T2) uses JPA to find entity Bug with id=1 and update it while T1 sleeps:

        int jpaQueryTimeoutMillis = 6000;
        //The QT is in units of milliseconds as per the spec:
        props.put("javax.persistence.query.timeout", jpaQueryTimeoutMillis);
        .....
        em.getTransaction().begin();
        Bug b = em.find(Bug.class, 1);
        b.setTitle("update by find");
        //The 'commit' will hang waiting for the lock on the row:
        em.getTransaction().commit();

        With this test, you can see that the QT is set to 6000ms and T1 will sleep for 9000ms, in other words T1 will sleep longer than QT. Because we are doing a find/update the QT will have no effect on the scenario (at least the allusion is it doesn't have an effect, as I'll explain in a moment). Therefore the outcome is that T2 will wait until it gets the row lock (after about 9000ms) and after it gets the lock it will successfully update the entity. If on the other hand, you change the find/update to something like the follow, On OpenJPA (but not ECL due to bug Bug 456067 ) would get a query timeout type of exception (QTE) after 6000ms:

        Query q = em.createQuery("UPDATE Bug o SET o.title = 'my title' where o.bugId = 1");
        q.executeUpdate();
        em.getTransaction().commit();

        So, at this time, all is as expected (assume we all agree that the QT should NOT have an effect on the find/update scenario) - expect for ECL in the query case due to Bug 456067 , however if I update the test to take into account ECL uses seconds I can get a QTE in the ECL case, so once ECL fixes the bug all will be the same for OJ and ECL.
        Next, I will explain why the scenario is giving the "allusion" that the find/update is not effected by the QT. In the above find/update scenario, change 'jdbcDelayTime' from 9000ms to say 9000000ms (i.e. 9000 seconds). Leave QT at 6000ms. In this case, we will get a QTE after approx. 6000 seconds. Yes, 6000 seconds, NOT milliseconds. In the Query scenario, the results are the same, we'd get a QTE after 6 seconds. Therefore, for a Query, life is good and the QT is behaving properly. However, for the find/update scenario, I think we have to ask ourselves two questions:

        1) Why is the QT having an effect on the find/update (and should it)?
        2) Why is the QT being treated as seconds, rather than ms, in the find/update case.

        For #1, I won't go into a ton of details at this time since this note is getting so long, but basically 'ConfiguringConnectionDecorator' does a 'setQueryTimeout' during the prepare and create of a statement. So the timeout would seem to be set on the statement ALWAYS, regardless of whether a query is performed or find/update. Furthermore, the value is not divided by 1000 (i.e. not converted from ms, to seconds, so the QT value is treated as seconds in this case). This would explain why there is the 'allusion' that the QT has no effect on a find/update scenario when the QT is larger than the JDBC sleep time in my scenario.....because we think we are setting the QT in millis, yet seconds are actually used by the JDBC driver, it seems like the ST has no effect......it is only when the QT is less than the JDBC sleep time that we see the QT does actually have an effect on the find/update case. Now, in the case of a Query, there are places where we call 'setQueryTimeout' on the Statement a seconds time. One such place is the JDBCStoreQuery which calls the dictionary which in turn calls 'setQueryTimeout'. In this case, the QT is divided by 1000 to convert the JPA spec defined QT units of millis to the JDBC expected units of seconds. Therefore, in the query case, the QT is properly handled (well at least for OJ, not for ECL).
        In conclusion, I'd say we have a bug here either way and one of the following options needs to be taken (for OpenJPA):
        a) if the QT should not effect the find/update, then we need to detect this path and not call 'setQueryTimeout' in the 'ConfiguringConnectionDecorator'.
        b) If the QT should effect the find/update, then we need to convert from millis to seconds.

        Phew.....thoughts? I'd be glad to discuss this on the phone if that would be easier.

        Thanks,

        Heath
        >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>EMAIL END>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

        Thanks,

        Heath Thomann

        Show
        jpaheath Heath Thomann added a comment - OK, I have talked to Rick Curtis and Kevin Sutter about this some more and I have to add some more details and slightly back track. This post is going to be extremely long, however, I want to add lots of details for history sake. In this post I'm also going to do something that is probably very unorthodox, and that is I'm going to explain the way EclipseLink (ECL) handles the 'javax.persistence.query.timeout' (QT for short). I know, I know, I shouldn't mix the two, however it appears that two major JPA providers are incorrectly handling the QT so I think it is worth pointing this out as it may also help to justify our final proposed handling of QT. I'd suggest the reader read the email I sent to Rick and Kevin for back ground [1] . This email details what OpenJPA is doing wrong when it comes to handling of the QT during a find/update scenario. Basically OpenJPA does two things wrong: 1) It applies the "javax.persistence.query.timeout" value to a find/update (an EM operation). 2) It uses units of seconds, rather than milliseconds, in the find/update case. As I explained back on May 27th, the spec is slightly vague about the application of QT. But after a careful read, you can see that the QT should NOT apply to EM operations, only Query operations. Therefore, #1 by OpenJPA (and ECL) is incorrect. However, given that OpenJPA has applied the QT to EM operations since the inception of QT, we feel it is best to continue to do so. We do not what to fix this and break all users who may directly or indirectly rely on this. At some point we could add options to enable/disable this (although OpenJPA does allow a user to disable the QT.....actually this is the default value). Also keep in mind that the JPA 2.0 and 2.1 specs both point out that the QT is just a hint, and a user should not completely depend on it, so I think there is further wiggle room for interpretation and implementation. Furthermore, customer's have argued that the QT should apply to all operations; both EM and Query. IMHO I agree with them.....it seems odd that one can control a timeout for a query, but NOT for an exactly same find/update operation. Next, given that we are not going to change things, we do feel we should address #2. We do feel we should do as the originator of this JIRA suggested, that is, to properly convert the QT value in 'ConfiguringConnectionDecorator' to milliseconds, from seconds. The DBDictionary converts the QT from milliseconds to seconds, the 'ConfiguringConnectionDecorator' should follow suite for consistency. [1] Email to Kevin Sutter, Rick Curtis, et al.: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>EMAIL START>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Hey Team! I need help with a likely bug in OpenJPA (and ECL?) with our javax.persistence.query.timeout (QT for short) usage. I know some of you worked on a QT issue on ECL.....my issue might also apply to ECL. This issue takes a bit of time to describe so settle in with a cup of coffee or tea. Basically though the root question is: should the QT apply to a finder+update (EM operation, no Query)? If so both OJ and ECL have a bug. To start, you could read my details in OJ2517, and look at the test. But even these details aren't exactly the same as my latest issue. In the JIRA the user was expecting the QT to apply to a EM.find/update scenario. In other words, they expected the QT to apply to ALL EM operations. However, as I explained in the JIRA the QT should only apply to javax.persistence.Query operations......Kevin's email [removed] seemed to back this up, the spec 'implies' this, and the TestQueryTimout test written by Pinaki would seem to offer further proof. However, I've found a scenario where both OJ and ECL apply the QT to an EM.find/update. Let me explain exactly what I mean by a 'EM.find/update' by using a code snippet to describe the scenario: Thread 1 (T1) uses JDBC to 'lock' a row with id 1, in a table name Bug, and then sleeps, as follows: int jdbcDelayTime = 9000; Connection conn = ....... Statement stmt = conn.createStatement(); stmt.execute("UPDATE Bug set title = 'a' where bugId = 1"); System.out.println("\nJDBC Thread will now sleep for " + jdbcDelayTime + " ms"); // emulate long process to hold lock on the row: Thread.sleep(jdbcDelayTime); Thread 2 (T2) uses JPA to find entity Bug with id=1 and update it while T1 sleeps: int jpaQueryTimeoutMillis = 6000; //The QT is in units of milliseconds as per the spec: props.put("javax.persistence.query.timeout", jpaQueryTimeoutMillis); ..... em.getTransaction().begin(); Bug b = em.find(Bug.class, 1); b.setTitle("update by find"); //The 'commit' will hang waiting for the lock on the row: em.getTransaction().commit(); With this test, you can see that the QT is set to 6000ms and T1 will sleep for 9000ms, in other words T1 will sleep longer than QT. Because we are doing a find/update the QT will have no effect on the scenario (at least the allusion is it doesn't have an effect, as I'll explain in a moment). Therefore the outcome is that T2 will wait until it gets the row lock (after about 9000ms) and after it gets the lock it will successfully update the entity. If on the other hand, you change the find/update to something like the follow, On OpenJPA (but not ECL due to bug Bug 456067 ) would get a query timeout type of exception (QTE) after 6000ms: Query q = em.createQuery("UPDATE Bug o SET o.title = 'my title' where o.bugId = 1"); q.executeUpdate(); em.getTransaction().commit(); So, at this time, all is as expected (assume we all agree that the QT should NOT have an effect on the find/update scenario) - expect for ECL in the query case due to Bug 456067 , however if I update the test to take into account ECL uses seconds I can get a QTE in the ECL case, so once ECL fixes the bug all will be the same for OJ and ECL. Next, I will explain why the scenario is giving the "allusion" that the find/update is not effected by the QT. In the above find/update scenario, change 'jdbcDelayTime' from 9000ms to say 9000000ms (i.e. 9000 seconds). Leave QT at 6000ms. In this case, we will get a QTE after approx. 6000 seconds. Yes, 6000 seconds, NOT milliseconds. In the Query scenario, the results are the same, we'd get a QTE after 6 seconds. Therefore, for a Query, life is good and the QT is behaving properly. However, for the find/update scenario, I think we have to ask ourselves two questions: 1) Why is the QT having an effect on the find/update (and should it)? 2) Why is the QT being treated as seconds, rather than ms, in the find/update case. For #1, I won't go into a ton of details at this time since this note is getting so long, but basically 'ConfiguringConnectionDecorator' does a 'setQueryTimeout' during the prepare and create of a statement. So the timeout would seem to be set on the statement ALWAYS, regardless of whether a query is performed or find/update. Furthermore, the value is not divided by 1000 (i.e. not converted from ms, to seconds, so the QT value is treated as seconds in this case). This would explain why there is the 'allusion' that the QT has no effect on a find/update scenario when the QT is larger than the JDBC sleep time in my scenario.....because we think we are setting the QT in millis, yet seconds are actually used by the JDBC driver, it seems like the ST has no effect......it is only when the QT is less than the JDBC sleep time that we see the QT does actually have an effect on the find/update case. Now, in the case of a Query, there are places where we call 'setQueryTimeout' on the Statement a seconds time. One such place is the JDBCStoreQuery which calls the dictionary which in turn calls 'setQueryTimeout'. In this case, the QT is divided by 1000 to convert the JPA spec defined QT units of millis to the JDBC expected units of seconds. Therefore, in the query case, the QT is properly handled (well at least for OJ, not for ECL). In conclusion, I'd say we have a bug here either way and one of the following options needs to be taken (for OpenJPA): a) if the QT should not effect the find/update, then we need to detect this path and not call 'setQueryTimeout' in the 'ConfiguringConnectionDecorator'. b) If the QT should effect the find/update, then we need to convert from millis to seconds. Phew.....thoughts? I'd be glad to discuss this on the phone if that would be easier. Thanks, Heath >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>EMAIL END>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks, Heath Thomann
        Hide
        mskb Masafumi Koba added a comment -

        Heath, Thank you for a detailed report.

        As you pointed out, I misunderstood that 'javax.persistence.query.timeout' will be applied also in 'EntitiManger.persist' method.

        I think that there is no problem with closing this issue.

        Thanks.

        Show
        mskb Masafumi Koba added a comment - Heath, Thank you for a detailed report. As you pointed out, I misunderstood that 'javax.persistence.query.timeout' will be applied also in 'EntitiManger.persist' method. I think that there is no problem with closing this issue. Thanks.
        Hide
        jpaheath Heath Thomann added a comment -

        HI! I've been looking at this issue and running my findings by Rick Curtis. I'll detail my findings, but basically OpenJPA already converts the query timeout from milliseconds to seconds, so things are working as expected.....I think the expectations in the test are incorrect.

        I think we have a couple of issues going on:
        1) We missed the fact that OpenJPA code does actually convert the timeout to seconds (see DBDictionary.setStatementQueryTimeout) so it is not needed as is proposed in the fix attached to this JIRA.
        2) The originator (and those of us initially looking at this JIRA) missed the fact that a 'persist' is used (rather than a query) in the provided test and as such are expecting a query timeout to apply to EM operations.

        Let me touch on each of these: First, if you look at TestQueryTimeout [1] you can see that the query timeout is very extensively tested. So I think this property is tested well and working well. Next, I found that OpenJPA does convert the timeout from milliseconds to seconds, see here in DBDictionary.setStatementQueryTimeout:

        /**

        • Allow subclasses to provide DB unique override implementations of
        • setting query timeouts, while preserving the default timeout logic
        • in the public setQueryTimeout method.
        • @param stmnt
        • @param timeout in milliseconds
        • @throws SQLException
          */
          protected void setStatementQueryTimeout(PreparedStatement stmnt,
          int timeout) throws SQLException { // JDBC uses seconds, so we'll do a simple round-down conversion here stmnt.setQueryTimeout(timeout / 1000); }

        Given this, I decided to run the test sent into this JIRA. In doing so, it finally hit me that the test is doing a 'persist' while some other thread has locked the same row in the DB. So the user is expecting the query timeout to apply to all EM operations. However, the query timeout should only apply to a query (the spec implies this). Again, if you look at TestQueryTimeout, you'll see a test method with comments:

        /**

        • Scenario being tested: 6) PU Query timeout hints do not affect EM
        • operations like updating Entities returned by EM.find()/findAll()
        • Expected Results: The DELAY function is being called and the update
        • takes 2000+ msecs to complete.
          */
          public void testQueryTimeout6() {

        In other words, this method verifies that the 'find', which takes longer than the query timeout, doesn't time out. So I believe the intentions are to not apply the query timeout to EM find, etc.

        I can understand the confusion here because the Spec is not 100% clear here. I think a person needs to read the spec closely on this property. Let me point out a few entries which imply the query timeout is limited to the Query interface (and queries). First, the javax.persistence.query.timeout is defined here:

        >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
        3.8.9 Query Hints
        The following hint is defined by this specification for use in query configuration.

        javax.persistence.query.timeout // time in milliseconds

        This hint may be used with the Query or TypedQuery setHint method or the NamedQuery and
        NamedNativeQuery annotations. It may also be passed as a property to the Persistence.createEntityManagerFactory
        method and used in the properties element of the persistence.
        xml file.
        >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

        This appears to be referred to as the "query timeout" throughout the rest of the spec. Next, If you look at the Query interface (section 3.8.1), it makes reference to a "query timeout".....the other hits of "query timeout" throughout the spec deal with the Query interface or query operations. The methods on the EM interface talk about a "timeout", but never a "query timeout".
        To me, this implies that the query.timeout only applies to the Query methods.

        Finally, in working with the test of OJ2517, I added a test where a query is used and verified correctness of the query timeout. The test works as I'd expect for Derby and H2 (I choose H2 because that is the database in use in the original test sent in). Let me attach my test, named openjpa-querytimeout-working.zip in case someone wants to look at it or try it out.

        I propose this JIRA be closed if folks agree with my findings.

        Thanks,

        Heath Thomann

        [1] https://fisheye6.atlassian.com/browse/openjpa/branches/2.2.x/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/query/TestQueryTimeout.java?r=1244807

        Show
        jpaheath Heath Thomann added a comment - HI! I've been looking at this issue and running my findings by Rick Curtis. I'll detail my findings, but basically OpenJPA already converts the query timeout from milliseconds to seconds, so things are working as expected.....I think the expectations in the test are incorrect. I think we have a couple of issues going on: 1) We missed the fact that OpenJPA code does actually convert the timeout to seconds (see DBDictionary.setStatementQueryTimeout) so it is not needed as is proposed in the fix attached to this JIRA. 2) The originator (and those of us initially looking at this JIRA) missed the fact that a 'persist' is used (rather than a query) in the provided test and as such are expecting a query timeout to apply to EM operations. Let me touch on each of these: First, if you look at TestQueryTimeout [1] you can see that the query timeout is very extensively tested. So I think this property is tested well and working well. Next, I found that OpenJPA does convert the timeout from milliseconds to seconds, see here in DBDictionary.setStatementQueryTimeout: /** Allow subclasses to provide DB unique override implementations of setting query timeouts, while preserving the default timeout logic in the public setQueryTimeout method. @param stmnt @param timeout in milliseconds @throws SQLException */ protected void setStatementQueryTimeout(PreparedStatement stmnt, int timeout) throws SQLException { // JDBC uses seconds, so we'll do a simple round-down conversion here stmnt.setQueryTimeout(timeout / 1000); } Given this, I decided to run the test sent into this JIRA. In doing so, it finally hit me that the test is doing a 'persist' while some other thread has locked the same row in the DB. So the user is expecting the query timeout to apply to all EM operations. However, the query timeout should only apply to a query (the spec implies this). Again, if you look at TestQueryTimeout, you'll see a test method with comments: /** Scenario being tested: 6) PU Query timeout hints do not affect EM operations like updating Entities returned by EM.find()/findAll() Expected Results: The DELAY function is being called and the update takes 2000+ msecs to complete. */ public void testQueryTimeout6() { In other words, this method verifies that the 'find', which takes longer than the query timeout, doesn't time out. So I believe the intentions are to not apply the query timeout to EM find, etc. I can understand the confusion here because the Spec is not 100% clear here. I think a person needs to read the spec closely on this property. Let me point out a few entries which imply the query timeout is limited to the Query interface (and queries). First, the javax.persistence.query.timeout is defined here: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3.8.9 Query Hints The following hint is defined by this specification for use in query configuration. javax.persistence.query.timeout // time in milliseconds This hint may be used with the Query or TypedQuery setHint method or the NamedQuery and NamedNativeQuery annotations. It may also be passed as a property to the Persistence.createEntityManagerFactory method and used in the properties element of the persistence. xml file. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This appears to be referred to as the "query timeout" throughout the rest of the spec. Next, If you look at the Query interface (section 3.8.1), it makes reference to a "query timeout".....the other hits of "query timeout" throughout the spec deal with the Query interface or query operations. The methods on the EM interface talk about a "timeout", but never a "query timeout". To me, this implies that the query.timeout only applies to the Query methods. Finally, in working with the test of OJ2517, I added a test where a query is used and verified correctness of the query timeout. The test works as I'd expect for Derby and H2 (I choose H2 because that is the database in use in the original test sent in). Let me attach my test, named openjpa-querytimeout-working.zip in case someone wants to look at it or try it out. I propose this JIRA be closed if folks agree with my findings. Thanks, Heath Thomann [1] https://fisheye6.atlassian.com/browse/openjpa/branches/2.2.x/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/query/TestQueryTimeout.java?r=1244807
        Hide
        kwsutter Kevin Sutter added a comment -

        Just to be clear... We would change the OpenJPA default processing to be compliant with the spec, and introduce the compatibility switch to use the wrong behavior. I'm sure Rick understood that approach, but I wanted to be clear in the JIRA.

        Show
        kwsutter Kevin Sutter added a comment - Just to be clear... We would change the OpenJPA default processing to be compliant with the spec, and introduce the compatibility switch to use the wrong behavior. I'm sure Rick understood that approach, but I wanted to be clear in the JIRA.
        Hide
        mskb Masafumi Koba added a comment -

        I have a good understanding of the root of this problem, and also compatibility issues.

        I agree with an idea of compatibility switch.

        Show
        mskb Masafumi Koba added a comment - I have a good understanding of the root of this problem, and also compatibility issues. I agree with an idea of compatibility switch.
        Hide
        curtisr7 Rick Curtis added a comment -

        I dug into this one a bit and it appears that the root issue is that when we implemented this support, it was against a non-final version of the JPA-2.0 spec. If you take a look at OPENJPA-878, you'll see the following comment :

        Support default query hint for query timeout as defined in section 3.6.4 of the spec.
        A new hint can be supplied for Java SE and Java EE environments -
        javax.persistence.query.timeout // query timeout in seconds

        I looked back through some non-final revisions of the spec, and this property did indeed change from seconds to milliseconds.

        That being said, I'm somewhat afraid to just fix this problem as users of OpenJPA could be unknowingly relying on this behavior(bug). In addition to fixing the bug, I believe that we'll need to add a compatibility switch to support running with javax.persistence.query.timeout as seconds.

        Show
        curtisr7 Rick Curtis added a comment - I dug into this one a bit and it appears that the root issue is that when we implemented this support, it was against a non-final version of the JPA-2.0 spec. If you take a look at OPENJPA-878 , you'll see the following comment : Support default query hint for query timeout as defined in section 3.6.4 of the spec. A new hint can be supplied for Java SE and Java EE environments - javax.persistence.query.timeout // query timeout in seconds I looked back through some non-final revisions of the spec, and this property did indeed change from seconds to milliseconds. That being said, I'm somewhat afraid to just fix this problem as users of OpenJPA could be unknowingly relying on this behavior(bug). In addition to fixing the bug, I believe that we'll need to add a compatibility switch to support running with javax.persistence.query.timeout as seconds.
        Hide
        mskb Masafumi Koba added a comment -

        Thank you, Rick.

        The release of my project is planned still ahead, so I don't immediately need this fix.

        Show
        mskb Masafumi Koba added a comment - Thank you, Rick. The release of my project is planned still ahead, so I don't immediately need this fix.
        Hide
        curtisr7 Rick Curtis added a comment -

        I took a look at your fix and it looks good. I'll try to take some time to work this into trunk sometime later this week. Is there a particular release that you need this fix committed into?

        Show
        curtisr7 Rick Curtis added a comment - I took a look at your fix and it looks good. I'll try to take some time to work this into trunk sometime later this week. Is there a particular release that you need this fix committed into?
        Hide
        mskb Masafumi Koba added a comment - - edited

        Attached Maven project for bug reproduction (openjpa-querytimeout-bug.zip).
        Please run command 'mvn clean test'.

        Show
        mskb Masafumi Koba added a comment - - edited Attached Maven project for bug reproduction (openjpa-querytimeout-bug.zip). Please run command 'mvn clean test'.

          People

          • Assignee:
            jpaheath Heath Thomann
            Reporter:
            mskb Masafumi Koba
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development