JDO
  1. JDO
  2. JDO-623

Query cancel and timeout support

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: JDO 3 (3.0)
    • Component/s: api, tck
    • Labels:
      None

      Description

      JDO doesn't have a mechanism to stop queries from overrunning. JPA2 now allows
      a persistence property to allow timing them out, and most JDO implementations
      have allowed this as an extension since JDO1. It would make sense for JDO
      (2.3) to have the same or a variation. I propose having the following

      Simple PMF property "javax.jdo.option.queryTimeout" to specify the number of millisecs (or secs) before any query is timed out. Throw a QueryTimeoutException (extends JDOException) when the timeout happens.

      Add methods Query.setTimeout(int), Query.getTimeout() to allow setting/retrieving the timeout interval on a per-query basis.

      Add method Query.cancel() to cancel any running query. If an implementation doesn't support cancelling of queries then it should throw a JDOUnsupportedOptionException. Any query execute() that is cancelled will throw a QueryInterruptedException (extends JDOUserException).

      1. jdo623.patch
        12 kB
        Craig L Russell
      2. JDO-623-ZeroTimeoutTestcase.patch
        14 kB
        Michael Bouschen
      3. JDO-623-testcase.patch
        26 kB
        Michael Bouschen
      4. jdo623.patch
        14 kB
        Craig L Russell
      5. JDO-623-tck2-mbo.patch
        15 kB
        Michael Bouschen
      6. pmf_option.patch
        0.7 kB
        Andy Jefferson
      7. query.patch
        1 kB
        Andy Jefferson
      8. JDO-623-mbo.patch
        3 kB
        Michael Bouschen
      9. query_timeout.patch
        10 kB
        Andy Jefferson

        Activity

        Hide
        Andy Jefferson added a comment -

        What about pm.conf "DatastoreReadTimeout" in the use of timeout within a query ? It expects a JDODataStoreException within 10ms. The PreparedStatement has a setQueryTimeout taking in *seconds*, hence is unusable for such control. DataNucleus also starts the datastore execution in a FutureTask and to wait for this 10ms, sadly the control goes back to the test before this is triggered. I fail to see what more an implementation can do, apart from throw an Unsupported exception when the timeout is lower than 1 second.

        Show
        Andy Jefferson added a comment - What about pm.conf "DatastoreReadTimeout" in the use of timeout within a query ? It expects a JDODataStoreException within 10ms. The PreparedStatement has a setQueryTimeout taking in * seconds *, hence is unusable for such control. DataNucleus also starts the datastore execution in a FutureTask and to wait for this 10ms, sadly the control goes back to the test before this is triggered. I fail to see what more an implementation can do, apart from throw an Unsupported exception when the timeout is lower than 1 second.
        Hide
        Michael Bouschen added a comment -

        Assertion seems to be untestable and thus I drop it from query.conf.

        Show
        Michael Bouschen added a comment - Assertion seems to be untestable and thus I drop it from query.conf.
        Hide
        Andy Jefferson added a comment -

        Rerun test. Still fails.
        SELECT AVG((A0.X + VAR_POINT2.Y)) FROM DATASTOREIDENTITY0.PCPOINT A0,DATASTOREIDENTITY0.PCPOINT2 VAR_POINT2 WHERE A0.Y >= 0 AND VAR_POINT2.X >= 0
        Execution Time = 44 ms

        If you want to try it yourself, use DN nightly jars 2.1.0-m1-SNAPSHOT of "datanucleus-core" and "datanucleus-rdbms".

        PS. Derby does some weird stuff in the JDBC call ResultSet.next() which hangs for 10seconds or so on this query. Presumably it evaluates the AVG of that query at that point, so hence the actual query execution is "fast", and hence a timeout is pointless.

        Show
        Andy Jefferson added a comment - Rerun test. Still fails. SELECT AVG((A0.X + VAR_POINT2.Y)) FROM DATASTOREIDENTITY0.PCPOINT A0,DATASTOREIDENTITY0.PCPOINT2 VAR_POINT2 WHERE A0.Y >= 0 AND VAR_POINT2.X >= 0 Execution Time = 44 ms If you want to try it yourself, use DN nightly jars 2.1.0-m1-SNAPSHOT of "datanucleus-core" and "datanucleus-rdbms". PS. Derby does some weird stuff in the JDBC call ResultSet.next() which hangs for 10seconds or so on this query. Presumably it evaluates the AVG of that query at that point, so hence the actual query execution is "fast", and hence a timeout is pointless.
        Hide
        Michael Bouschen added a comment -

        According to Andy the QueryCancel class tries to cancel the query that it hasn't yet started.

        I changed the test class:

        • It creates 5000 PCPoint and PCPoint2 instances (instead of 1000). This should make the join (as part of the query) more expensive such that the query takes longer to execute.
        • It uses a barrier to synchronize the threads
        • It compiles the query before staring the thread

        Andy,
        could you please rerun the test class with the above changes? Thanks!

        Show
        Michael Bouschen added a comment - According to Andy the QueryCancel class tries to cancel the query that it hasn't yet started. I changed the test class: It creates 5000 PCPoint and PCPoint2 instances (instead of 1000). This should make the join (as part of the query) more expensive such that the query takes longer to execute. It uses a barrier to synchronize the threads It compiles the query before staring the thread Andy, could you please rerun the test class with the above changes? Thanks!
        Hide
        Craig L Russell added a comment -

        Updated constants and signatures.

        svn commit -m "JDO-623 Update signatures and constants for Datastore timeout" api2/src tck2/src/conf tck2/src/java/org/apache/jdo/tck/JDO_Test.java tck2/src/java/org/apache/jdo/tck/api/persistencemanager/DatastoreTimeout.java
        Sending api2/src/java/javax/jdo/Constants.java
        Sending api2/src/java/javax/jdo/JDOHelper.java
        Sending api2/src/schema/javax/jdo/jdoconfig_2_3.dtd
        Sending api2/src/schema/javax/jdo/jdoconfig_2_3.xsd
        Sending api2/src/schema/javax/jdo/jdoquery_2_3.dtd
        Sending api2/src/schema/javax/jdo/jdoquery_2_3.xsd
        Sending tck2/src/conf/jdo-2_3-signatures.txt
        Sending tck2/src/java/org/apache/jdo/tck/JDO_Test.java
        Sending tck2/src/java/org/apache/jdo/tck/api/persistencemanager/DatastoreTimeout.java
        Transmitting file data .........
        Committed revision 916585.

        Show
        Craig L Russell added a comment - Updated constants and signatures. svn commit -m " JDO-623 Update signatures and constants for Datastore timeout" api2/src tck2/src/conf tck2/src/java/org/apache/jdo/tck/JDO_Test.java tck2/src/java/org/apache/jdo/tck/api/persistencemanager/DatastoreTimeout.java Sending api2/src/java/javax/jdo/Constants.java Sending api2/src/java/javax/jdo/JDOHelper.java Sending api2/src/schema/javax/jdo/jdoconfig_2_3.dtd Sending api2/src/schema/javax/jdo/jdoconfig_2_3.xsd Sending api2/src/schema/javax/jdo/jdoquery_2_3.dtd Sending api2/src/schema/javax/jdo/jdoquery_2_3.xsd Sending tck2/src/conf/jdo-2_3-signatures.txt Sending tck2/src/java/org/apache/jdo/tck/JDO_Test.java Sending tck2/src/java/org/apache/jdo/tck/api/persistencemanager/DatastoreTimeout.java Transmitting file data ......... Committed revision 916585.
        Hide
        Michael Bouschen added a comment -

        The patch looks good!

        Show
        Michael Bouschen added a comment - The patch looks good!
        Hide
        Craig L Russell added a comment -

        Please review this patch.

        I found a few places in the api and tck which referenced the query timeout versus datastore timeout.

        I also found a few configuration properties that needed to be added to jdoquery and jdoconfig definitions.

        Show
        Craig L Russell added a comment - Please review this patch. I found a few places in the api and tck which referenced the query timeout versus datastore timeout. I also found a few configuration properties that needed to be added to jdoquery and jdoconfig definitions.
        Hide
        Michael Bouschen added a comment -

        Checked in the patch (see revision 904863).

        Show
        Michael Bouschen added a comment - Checked in the patch (see revision 904863).
        Hide
        Michael Bouschen added a comment -

        Please review the attached patch JDO-623-ZeroTimeoutTestcase.patch.
        It adds to new test methods to TCK class DatastoreTimeout, testing setting the timeout value to 0.

        Show
        Michael Bouschen added a comment - Please review the attached patch JDO-623 -ZeroTimeoutTestcase.patch. It adds to new test methods to TCK class DatastoreTimeout, testing setting the timeout value to 0.
        Hide
        Michael Bouschen added a comment -

        Reopend, because I want to add more test methods testing zero timeouts.

        Show
        Michael Bouschen added a comment - Reopend, because I want to add more test methods testing zero timeouts.
        Hide
        Craig L Russell added a comment -

        The JDO trunk now passes on DataNucleus 2.0.1-SNAPSHOT.

        All that's left is the specification update.

        Show
        Craig L Russell added a comment - The JDO trunk now passes on DataNucleus 2.0.1-SNAPSHOT. All that's left is the specification update.
        Hide
        Andy Jefferson added a comment -

        JDO TCK SVN will look for datanucleus-core/datanucleus-jpa 2.0.1-SNAPSHOT. They are in the DN M2 nightly repo (and also the DN M1 nightly repo I'd guess). Just running Maven not "offline" should find them.

        Show
        Andy Jefferson added a comment - JDO TCK SVN will look for datanucleus-core/datanucleus-jpa 2.0.1-SNAPSHOT. They are in the DN M2 nightly repo (and also the DN M1 nightly repo I'd guess). Just running Maven not "offline" should find them.
        Hide
        Craig L Russell added a comment -

        What's the maven incantation to get the current DN SVN?

        Show
        Craig L Russell added a comment - What's the maven incantation to get the current DN SVN?
        Hide
        Andy Jefferson added a comment -

        Unassigned since the test passes with current DN SVN. If further is required on this JIRA then someone take ownership of it

        Show
        Andy Jefferson added a comment - Unassigned since the test passes with current DN SVN. If further is required on this JIRA then someone take ownership of it
        Hide
        Michael Bouschen added a comment -

        Checked in the patch (see revision 898066).

        Assigning the issue to Andy, because the testcase fails with the current datanucleus version. I think this is because method PMF.supportedOptions does not include "javax.jdo.option.DatastoreTimeout" in the returned collection. So the testcase assumes datastore timeout is not supported and thus expects the setter methods setDatastoreRead/WriteTimeout should result in a JDOUnsupportedOptionException.

        Show
        Michael Bouschen added a comment - Checked in the patch (see revision 898066). Assigning the issue to Andy, because the testcase fails with the current datanucleus version. I think this is because method PMF.supportedOptions does not include "javax.jdo.option.DatastoreTimeout" in the returned collection. So the testcase assumes datastore timeout is not supported and thus expects the setter methods setDatastoreRead/WriteTimeout should result in a JDOUnsupportedOptionException.
        Hide
        Craig L Russell added a comment -

        Changes to remove the old query timeout test and the changes to the signatures look good. I've run the tck with this patch and no signature issues.

        testDatastoreWriteTimeout doesn't use the MAIN_SLEEP_MILLIS constant.

        When I run on today's RI SNAPSHOT, it takes 10 seconds for the first test and 8 seconds for the second test, which indicates that the timeout methods are not doing anything but the PMF method is returning the timeout option.

        [java] There were 2 failures:
        [java] 1) testDatastoreReadTimeout(org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout)junit.framework.AssertionFailedError: Assertion A?? (DatastoreTimeout) failed:
        [java] Query.setDatastoreReadTimeoutMillis should throw a JDOUnsupportedOptionException, if datastore timeout is not supported
        [java] at org.apache.jdo.tck.JDO_Test.fail(JDO_Test.java:682)
        [java] at org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout.runQueryReadingPCPointInstances(DatastoreTimeout.java:180)
        [java] at org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout.testDatastoreReadTimeout(DatastoreTimeout.java:96)
        [java] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        [java] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        [java] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        [java] at org.apache.jdo.tck.JDO_Test.runBare(JDO_Test.java:272)
        [java] at org.apache.jdo.tck.util.BatchTestRunner.doRun(BatchTestRunner.java:108)
        [java] at org.apache.jdo.tck.util.BatchTestRunner.start(BatchTestRunner.java:148)
        [java] at org.apache.jdo.tck.util.BatchTestRunner.main(BatchTestRunner.java:123)
        [java] 2) testDatastoreWriteTimeout(org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout)junit.framework.AssertionFailedError: Assertion A?? (DatastoreTimeout) failed:
        [java] PM.setDatastoreWriteTimeoutMillis should throw a JDOUnsupportedOptionException, if datastore timeout is not supported
        [java] at org.apache.jdo.tck.JDO_Test.fail(JDO_Test.java:682)
        [java] at org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout.runUpdatePCointInstance(DatastoreTimeout.java:273)
        [java] at org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout.testDatastoreWriteTimeout(DatastoreTimeout.java:126)
        [java] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        [java] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        [java] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        [java] at org.apache.jdo.tck.JDO_Test.runBare(JDO_Test.java:272)
        [java] at org.apache.jdo.tck.util.BatchTestRunner.doRun(BatchTestRunner.java:108)
        [java] at org.apache.jdo.tck.util.BatchTestRunner.start(BatchTestRunner.java:148)
        [java] at org.apache.jdo.tck.util.BatchTestRunner.main(BatchTestRunner.java:123)
        [java] FAILURES!!!

        Show
        Craig L Russell added a comment - Changes to remove the old query timeout test and the changes to the signatures look good. I've run the tck with this patch and no signature issues. testDatastoreWriteTimeout doesn't use the MAIN_SLEEP_MILLIS constant. When I run on today's RI SNAPSHOT, it takes 10 seconds for the first test and 8 seconds for the second test, which indicates that the timeout methods are not doing anything but the PMF method is returning the timeout option. [java] There were 2 failures: [java] 1) testDatastoreReadTimeout(org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout)junit.framework.AssertionFailedError: Assertion A?? (DatastoreTimeout) failed: [java] Query.setDatastoreReadTimeoutMillis should throw a JDOUnsupportedOptionException, if datastore timeout is not supported [java] at org.apache.jdo.tck.JDO_Test.fail(JDO_Test.java:682) [java] at org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout.runQueryReadingPCPointInstances(DatastoreTimeout.java:180) [java] at org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout.testDatastoreReadTimeout(DatastoreTimeout.java:96) [java] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) [java] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) [java] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) [java] at org.apache.jdo.tck.JDO_Test.runBare(JDO_Test.java:272) [java] at org.apache.jdo.tck.util.BatchTestRunner.doRun(BatchTestRunner.java:108) [java] at org.apache.jdo.tck.util.BatchTestRunner.start(BatchTestRunner.java:148) [java] at org.apache.jdo.tck.util.BatchTestRunner.main(BatchTestRunner.java:123) [java] 2) testDatastoreWriteTimeout(org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout)junit.framework.AssertionFailedError: Assertion A?? (DatastoreTimeout) failed: [java] PM.setDatastoreWriteTimeoutMillis should throw a JDOUnsupportedOptionException, if datastore timeout is not supported [java] at org.apache.jdo.tck.JDO_Test.fail(JDO_Test.java:682) [java] at org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout.runUpdatePCointInstance(DatastoreTimeout.java:273) [java] at org.apache.jdo.tck.api.persistencemanager.DatastoreTimeout.testDatastoreWriteTimeout(DatastoreTimeout.java:126) [java] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) [java] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) [java] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) [java] at org.apache.jdo.tck.JDO_Test.runBare(JDO_Test.java:272) [java] at org.apache.jdo.tck.util.BatchTestRunner.doRun(BatchTestRunner.java:108) [java] at org.apache.jdo.tck.util.BatchTestRunner.start(BatchTestRunner.java:148) [java] at org.apache.jdo.tck.util.BatchTestRunner.main(BatchTestRunner.java:123) [java] FAILURES!!!
        Hide
        Michael Bouschen added a comment -

        Please review the attached patch JDO-623-testcase.patch.

        Changes:

        • It replaces the testcase QueryTimeout by a new class DatastoreTimeout testing both datastore read/write timeout. The new testcase still needs an assertion id.
        • The patch also adds the new datastore timeout setter and getter methods to the signature test input file jdo-2_3-signatures.txt.
        Show
        Michael Bouschen added a comment - Please review the attached patch JDO-623 -testcase.patch. Changes: It replaces the testcase QueryTimeout by a new class DatastoreTimeout testing both datastore read/write timeout. The new testcase still needs an assertion id. The patch also adds the new datastore timeout setter and getter methods to the signature test input file jdo-2_3-signatures.txt.
        Hide
        Andy Jefferson added a comment -

        The patch to api2 changed the API that tck2 was building against so broke the compile. This is fixed by

        Sending src/java/org/apache/jdo/tck/query/api/QueryTimeout.java
        Transmitting file data .
        Committed revision 894049.

        Show
        Andy Jefferson added a comment - The patch to api2 changed the API that tck2 was building against so broke the compile. This is fixed by Sending src/java/org/apache/jdo/tck/query/api/QueryTimeout.java Transmitting file data . Committed revision 894049.
        Hide
        Craig L Russell added a comment -

        The latest patch has been checked in.

        svn commit -m "JDO-623 Change QueryTImeout to DatastoreTimeout"
        Sending api2/src/java/javax/jdo/Constants.java
        Deleting api2/src/java/javax/jdo/JDOQueryTimeoutException.java
        Sending api2/src/java/javax/jdo/PersistenceManager.java
        Sending api2/src/java/javax/jdo/PersistenceManagerFactory.java
        Sending api2/src/java/javax/jdo/Query.java
        Transmitting file data ....
        Committed revision 892317.

        Show
        Craig L Russell added a comment - The latest patch has been checked in. svn commit -m " JDO-623 Change QueryTImeout to DatastoreTimeout" Sending api2/src/java/javax/jdo/Constants.java Deleting api2/src/java/javax/jdo/JDOQueryTimeoutException.java Sending api2/src/java/javax/jdo/PersistenceManager.java Sending api2/src/java/javax/jdo/PersistenceManagerFactory.java Sending api2/src/java/javax/jdo/Query.java Transmitting file data .... Committed revision 892317.
        Hide
        Andy Jefferson added a comment -

        I added stubs in DataNucleus several days ago for these methods; untested, so suggest that you apply your patch

        Show
        Andy Jefferson added a comment - I added stubs in DataNucleus several days ago for these methods; untested, so suggest that you apply your patch
        Hide
        Michael Bouschen added a comment -

        I agree, the patch is ready to be committed.

        Show
        Michael Bouschen added a comment - I agree, the patch is ready to be committed.
        Hide
        Craig L Russell added a comment -

        Please review this patch.

        I've fixed a typo and added this to PersistenceManagerFactory getXXXTimeoutMillis:

        • If timeouts are not supported,this method will return null.

        I think this patch is ready to be committed.

        Show
        Craig L Russell added a comment - Please review this patch. I've fixed a typo and added this to PersistenceManagerFactory getXXXTimeoutMillis: If timeouts are not supported,this method will return null. I think this patch is ready to be committed.
        Hide
        Michael Bouschen added a comment -

        The new patch look good!

        I vote for option 1, meaning getDatastoreReadTimeoutMillis and getDatastoreWriteTimeoutMillis should return null ig timeouts are not supported.

        Show
        Michael Bouschen added a comment - The new patch look good! I vote for option 1, meaning getDatastoreReadTimeoutMillis and getDatastoreWriteTimeoutMillis should return null ig timeouts are not supported.
        Hide
        Craig L Russell added a comment -

        Please review this patch.

        Comments from Michael have been incorporated.

        One question: if timeouts are not supported, should getDatastoreReadTimeoutMillis and getDatastoreWriteTimeoutMillis return null or throw an exception?

        Show
        Craig L Russell added a comment - Please review this patch. Comments from Michael have been incorporated. One question: if timeouts are not supported, should getDatastoreReadTimeoutMillis and getDatastoreWriteTimeoutMillis return null or throw an exception?
        Hide
        Michael Bouschen added a comment -

        The patch jdo623.patch looks good.

        Just two comments:

        • The javadoc of methods setDatastoreReadTimeoutMillis and setDatastoreWriteTimeoutMillis in Query specifies that a JDODatastoreTimeoutException is thrown in case of a timeout. I think it should be a JDODatastoreException.
        • Please add javax.jdo.option.DatastoreTimeout to the javadoc of method PMF.supportedOptions.
        Show
        Michael Bouschen added a comment - The patch jdo623.patch looks good. Just two comments: The javadoc of methods setDatastoreReadTimeoutMillis and setDatastoreWriteTimeoutMillis in Query specifies that a JDODatastoreTimeoutException is thrown in case of a timeout. I think it should be a JDODatastoreException. Please add javax.jdo.option.DatastoreTimeout to the javadoc of method PMF.supportedOptions.
        Hide
        Craig L Russell added a comment -

        Please review this patch. It changes the methods get/setQueryTimeout to get/setDatastoreReadTimeout and adds get/setDatastoreWriteTimeout to Query, PersistenceManager, and PersistenceManagerFactory. It adds javax.jdo.option.DatastoreTimeout to Constants and removes JDOQueryTimeoutException.

        Show
        Craig L Russell added a comment - Please review this patch. It changes the methods get/setQueryTimeout to get/setDatastoreReadTimeout and adds get/setDatastoreWriteTimeout to Query, PersistenceManager, and PersistenceManagerFactory. It adds javax.jdo.option.DatastoreTimeout to Constants and removes JDOQueryTimeoutException.
        Hide
        Andy Jefferson added a comment -

        And does the test actually check if "javax.jdo.option.QueryCancel" is a valid PMF supported property ?
        We don't have a separate property for "timeout" and, if the underlying datastore doesn't support it, what is expected to happen? 11.1 of the spec says that if the underlying datastore doesnt support it then nothing is set; any such TCK tests should allow for this in their logic

        Show
        Andy Jefferson added a comment - And does the test actually check if "javax.jdo.option.QueryCancel" is a valid PMF supported property ? We don't have a separate property for "timeout" and, if the underlying datastore doesn't support it, what is expected to happen? 11.1 of the spec says that if the underlying datastore doesnt support it then nothing is set; any such TCK tests should allow for this in their logic
        Hide
        Michael Bouschen added a comment -

        Added QueryTimeout TCK test class to the query.conf test configuration (see revision 821093) . This test still fails because timeouts are not yet supported by the regular datanucleus JDOQL implementation.

        Show
        Michael Bouschen added a comment - Added QueryTimeout TCK test class to the query.conf test configuration (see revision 821093) . This test still fails because timeouts are not yet supported by the regular datanucleus JDOQL implementation.
        Hide
        Michael Bouschen added a comment -

        Added QueryCancel TCK test class to the query.conf query test configuration, because datanucleus now supports query cancelling.

        I will add QueryTimeout test class as soon as datanucleus supports query timeout.

        Show
        Michael Bouschen added a comment - Added QueryCancel TCK test class to the query.conf query test configuration, because datanucleus now supports query cancelling. I will add QueryTimeout test class as soon as datanucleus supports query timeout.
        Hide
        Michael Bouschen added a comment -

        Checked in the patch adding the TCK test cases (see revision 804355).

        I haven't added the two new test classes to the tck query.conf file. This needs to be done as soon as the datanucleus default JDOQL implementation supports query canceling. I keep the JIRA issue unresolved until then.

        Show
        Michael Bouschen added a comment - Checked in the patch adding the TCK test cases (see revision 804355). I haven't added the two new test classes to the tck query.conf file. This needs to be done as soon as the datanucleus default JDOQL implementation supports query canceling. I keep the JIRA issue unresolved until then.
        Hide
        Craig L Russell added a comment -

        Nice work as usual.

        Just one comment: there are TODO items that are apparently resolved already.

        Show
        Craig L Russell added a comment - Nice work as usual. Just one comment: there are TODO items that are apparently resolved already.
        Hide
        Michael Bouschen added a comment -

        Attached you find a patch for review adding two new tck2 test classes:

        • QueryCancel
        • QueryTimeout

        I haven't added the two new test classes to the tck conf files. The current datanuclues JDOQL implementation does not support query canceling and the alternate implementation of JDOQL that will eventually replace the current one is not yet finished.

        In order to include the test classes add the following lines to the property jdo.tck.classes in query.conf:
        org.apache.jdo.tck.query.api.QueryCancel \
        org.apache.jdo.tck.query.api.QueryTimeout

        Show
        Michael Bouschen added a comment - Attached you find a patch for review adding two new tck2 test classes: QueryCancel QueryTimeout I haven't added the two new test classes to the tck conf files. The current datanuclues JDOQL implementation does not support query canceling and the alternate implementation of JDOQL that will eventually replace the current one is not yet finished. In order to include the test classes add the following lines to the property jdo.tck.classes in query.conf: org.apache.jdo.tck.query.api.QueryCancel \ org.apache.jdo.tck.query.api.QueryTimeout
        Hide
        Andy Jefferson added a comment -

        Patches to Query and PersistenceManagerFactory were applied to SVN trunk.
        Remaining work is for tests.

        Show
        Andy Jefferson added a comment - Patches to Query and PersistenceManagerFactory were applied to SVN trunk. Remaining work is for tests.
        Hide
        Andy Jefferson added a comment -

        Patch for PMF property of supported option

        Show
        Andy Jefferson added a comment - Patch for PMF property of supported option
        Hide
        Andy Jefferson added a comment -

        Updated javadoc relative to previous patch to allow for case of cancel failing

        Show
        Andy Jefferson added a comment - Updated javadoc relative to previous patch to allow for case of cancel failing
        Hide
        Andy Jefferson added a comment -

        Patch to rename Query.cancel to Query.cancelAll, and add Query.cancel(Thread)

        Show
        Andy Jefferson added a comment - Patch to rename Query.cancel to Query.cancelAll, and add Query.cancel(Thread)
        Hide
        Michael Bouschen added a comment -

        In order to close this we need a TCK test for Query cancel and timeout support and this is a challenge:

        • The test case does not know whether the implementation supports Query.cancel. So do we need a PMF supported option (e.g. javax.jdo.option.CancelQuery) to test this?
        • What kind of query is guaranteed to run long enough such that a parallel thread is able to cancel it?
        • The same hold true for a method testing the query timeout.
        Show
        Michael Bouschen added a comment - In order to close this we need a TCK test for Query cancel and timeout support and this is a challenge: The test case does not know whether the implementation supports Query.cancel. So do we need a PMF supported option (e.g. javax.jdo.option.CancelQuery) to test this? What kind of query is guaranteed to run long enough such that a parallel thread is able to cancel it? The same hold true for a method testing the query timeout.
        Hide
        Matthew T. Adams added a comment -

        I think we need an overloaded method here, cancel(Thread), and possible an additional method, cancelAll().

        In the event that a user shares a single Query instance across multiple threads, each of which is calling execute() in parallel, there is no way for the thread calling cancel to communicate to the implementation exactly which thread's running query to cancel. This implies that there needs to be an overload that takes a Thread instance: cancel(Thread).

        This has implications on Query#execute(): the implementation must note which Thread a query execution is executing on. This would allow the user to call cancel() in the simple, more common use case of only one query execution thread, as there would be only one execution recorded, and it would be the one to be cancelled. However, in the more complicated use case of multiple, concurrent executions, cancel() could either cancel all current executions, or throw a JDOUserException indicating an ambiguous cancellation request. If we decide that the method should throw, then another convenient method would be cancelAll().

        Show
        Matthew T. Adams added a comment - I think we need an overloaded method here, cancel(Thread), and possible an additional method, cancelAll(). In the event that a user shares a single Query instance across multiple threads, each of which is calling execute() in parallel, there is no way for the thread calling cancel to communicate to the implementation exactly which thread's running query to cancel. This implies that there needs to be an overload that takes a Thread instance: cancel(Thread). This has implications on Query#execute(): the implementation must note which Thread a query execution is executing on. This would allow the user to call cancel() in the simple, more common use case of only one query execution thread, as there would be only one execution recorded, and it would be the one to be cancelled. However, in the more complicated use case of multiple, concurrent executions, cancel() could either cancel all current executions, or throw a JDOUserException indicating an ambiguous cancellation request. If we decide that the method should throw, then another convenient method would be cancelAll().
        Hide
        Andy Jefferson added a comment -

        Patch was applied to SVN trunk.

        Show
        Andy Jefferson added a comment - Patch was applied to SVN trunk.
        Hide
        Michael Bouschen added a comment -

        I would like to propose a few changes:
        (1) Add methods set/getTimeoutMillis to the PersistenceManage interface.
        (2) Methods set/getTimeoutMillis should use an Integer argument resp. return value instead of the primitive type value. Then the getter can return null in case no timeout is specified.
        (3) For completeness we should add the getter getTimeoutMillis to the Query interface.
        I added a patch JDO-623-mbo.patch for review implementing the above changes.

        Updated spec changes:

        11.1
        Add "Query Timeout" (bold heading)
        <spec>
        If the query timeout is specified the implementation will use the supplied value as a timeout for any queries where the underlying datastore supports it. The timeout is in milliseconds. The specified value is used as default for all PersistenceManager instances obtained from the factory. If it is not specified then the implementation won't set any timeout (unless specified on the PersistenceManager or on a query-by-query basis).
        </spec>

        11.1.1
        Add
        "javax.jdo.option.QueryTimeoutInMillis"

        12.6.3
        Add at the end of section 12.6.3
        <spec>
        If the query timeout is specified the implementation will use the supplied value as a timeout for any queries where the underlying datastore supports it. The timeout is in milliseconds. The specified value overrides PersistenceManagerFactory property value (see 11.1). It is used as default for all Query instances obtained from the factory. If it is not specified then the implementation won't set any timeout (unless specified on a query-by-query basis).
        </spec>

        14.6
        Add
        "Timeout/Cancel" (bold heading)
        void setTimeoutMillis(int interval);
        This method sets the timeout for this query (in milliseconds) where the underlying datastore supports it. If the datastore doesn't support timeouts then a JDOQueryTimeoutException is thrown. This overrides the PersistenceManager property value (see 12.6.3).

        void cancel();
        This method allows the current query to be cancelled (if executing). If the implementation does not support this operation (maybe due to lack of support in the underlying datastore) then JDOUnsupportedOptionException is thrown.

        Show
        Michael Bouschen added a comment - I would like to propose a few changes: (1) Add methods set/getTimeoutMillis to the PersistenceManage interface. (2) Methods set/getTimeoutMillis should use an Integer argument resp. return value instead of the primitive type value. Then the getter can return null in case no timeout is specified. (3) For completeness we should add the getter getTimeoutMillis to the Query interface. I added a patch JDO-623 -mbo.patch for review implementing the above changes. Updated spec changes: 11.1 Add "Query Timeout" (bold heading) <spec> If the query timeout is specified the implementation will use the supplied value as a timeout for any queries where the underlying datastore supports it. The timeout is in milliseconds. The specified value is used as default for all PersistenceManager instances obtained from the factory. If it is not specified then the implementation won't set any timeout (unless specified on the PersistenceManager or on a query-by-query basis). </spec> 11.1.1 Add "javax.jdo.option.QueryTimeoutInMillis" 12.6.3 Add at the end of section 12.6.3 <spec> If the query timeout is specified the implementation will use the supplied value as a timeout for any queries where the underlying datastore supports it. The timeout is in milliseconds. The specified value overrides PersistenceManagerFactory property value (see 11.1). It is used as default for all Query instances obtained from the factory. If it is not specified then the implementation won't set any timeout (unless specified on a query-by-query basis). </spec> 14.6 Add "Timeout/Cancel" (bold heading) void setTimeoutMillis(int interval); This method sets the timeout for this query (in milliseconds) where the underlying datastore supports it. If the datastore doesn't support timeouts then a JDOQueryTimeoutException is thrown. This overrides the PersistenceManager property value (see 12.6.3). void cancel(); This method allows the current query to be cancelled (if executing). If the implementation does not support this operation (maybe due to lack of support in the underlying datastore) then JDOUnsupportedOptionException is thrown.
        Hide
        Andy Jefferson added a comment -

        Suggested spec updates :-
        11.1
        Add "Query Timeout" (bold heading)
        <spec>
        If the query timeout is specified the implementation will use the supplied value as a timeout for any queries where the underlying datastore supports it. The timeout is in milliseconds. If it is not specified then the implementation won't set any timeout (unless specified on a query-by-query basis).
        </spec>

        11.1.1
        Add
        "javax.jdo.option.QueryTimeoutInMillis"

        14.6
        Add
        "Timeout/Cancel" (bold heading)
        void setTimeoutMillis(int interval);
        This method sets the timeout for this query (in milliseconds) where the underlying datastore supports it. If the datastore doesn't support timeouts then a JDOQueryTimeoutException is thrown. This overrides the PersistenceManagerFactory property value (see 11.1)

        void cancel();
        This method allows the current query to be cancelled (if executing). If the implementation does not support this operation (maybe due to lack of support in the underlying datastore) then JDOUnsupportedOptionException is thrown.

        Show
        Andy Jefferson added a comment - Suggested spec updates :- 11.1 Add "Query Timeout" (bold heading) <spec> If the query timeout is specified the implementation will use the supplied value as a timeout for any queries where the underlying datastore supports it. The timeout is in milliseconds. If it is not specified then the implementation won't set any timeout (unless specified on a query-by-query basis). </spec> 11.1.1 Add "javax.jdo.option.QueryTimeoutInMillis" 14.6 Add "Timeout/Cancel" (bold heading) void setTimeoutMillis(int interval); This method sets the timeout for this query (in milliseconds) where the underlying datastore supports it. If the datastore doesn't support timeouts then a JDOQueryTimeoutException is thrown. This overrides the PersistenceManagerFactory property value (see 11.1) void cancel(); This method allows the current query to be cancelled (if executing). If the implementation does not support this operation (maybe due to lack of support in the underlying datastore) then JDOUnsupportedOptionException is thrown.
        Hide
        Andy Jefferson added a comment -

        Changes are now in SVN, including Craigs method renaming including "Millis" for clarity

        Show
        Andy Jefferson added a comment - Changes are now in SVN, including Craigs method renaming including "Millis" for clarity
        Hide
        Craig L Russell added a comment -

        > Since Query.cancel() cannot be invoked from the thread which started the execution, should PM.setMultithreaded(true) be invoked ?

        No, multithreaded is intended to require that the implementation protect data structures from harm in case multiple threads are active in data structures visible to the application. Query.cancel doesn't operate on visible data structures.

        > If setMultithreaded(true) is mandatory, should cancel() throw an Exception if multithreaded flag is false?

        No.

        > Is it the case to handle Query.cancel() without multithreaded flag being true ?

        Yes. Since the thread that is stuck in a query can't be simultaneously modifying data structures, normal care is all that's needed by the implementation to avoid corruption.

        Show
        Craig L Russell added a comment - > Since Query.cancel() cannot be invoked from the thread which started the execution, should PM.setMultithreaded(true) be invoked ? No, multithreaded is intended to require that the implementation protect data structures from harm in case multiple threads are active in data structures visible to the application. Query.cancel doesn't operate on visible data structures. > If setMultithreaded(true) is mandatory, should cancel() throw an Exception if multithreaded flag is false? No. > Is it the case to handle Query.cancel() without multithreaded flag being true ? Yes. Since the thread that is stuck in a query can't be simultaneously modifying data structures, normal care is all that's needed by the implementation to avoid corruption.
        Hide
        Guido Anzuoni added a comment -

        Since Query.cancel() cannot be invoked from the thread which started the execution, should PM.setMultithreaded(true) be invoked ?
        If setMultithreaded(true) is mandatory, should cancel() throw an Exception if multithreaded flag is false?
        Is it the case to handle Query.cancel() without multithreaded flag being true ?

        Show
        Guido Anzuoni added a comment - Since Query.cancel() cannot be invoked from the thread which started the execution, should PM.setMultithreaded(true) be invoked ? If setMultithreaded(true) is mandatory, should cancel() throw an Exception if multithreaded flag is false? Is it the case to handle Query.cancel() without multithreaded flag being true ?
        Hide
        Craig L Russell added a comment -

        This is a good addition to the spec.

        Most pmf properties are capitalized, so the pmf property would be javax.jdo.option.QueryTimeout, as you have put into your patch.

        We have all of the standard properties also in the PMF API. Question for discussion: should we add get/setQueryTimeout to the API?

        If the user or administrator cancels a query via an external (e.g. JDBC) API, should the same QueryInterruptus exception be thrown?

        Milliseconds is a good unit of measuremant for the timeout. Many drivers support millis, and the JDBC expert group seems to be moving in that direction. My biggest concern is usability, so I might even consider QueryTimeoutMillis to make is very clear that it's millis. And Query.get/setTimeoutMillis, PMF.get/setQueryTimeoutMillis. I'd draw the line at QueryInterruptedMillis though.

        Show
        Craig L Russell added a comment - This is a good addition to the spec. Most pmf properties are capitalized, so the pmf property would be javax.jdo.option.QueryTimeout, as you have put into your patch. We have all of the standard properties also in the PMF API. Question for discussion: should we add get/setQueryTimeout to the API? If the user or administrator cancels a query via an external (e.g. JDBC) API, should the same QueryInterruptus exception be thrown? Milliseconds is a good unit of measuremant for the timeout. Many drivers support millis, and the JDBC expert group seems to be moving in that direction. My biggest concern is usability, so I might even consider QueryTimeoutMillis to make is very clear that it's millis. And Query.get/setTimeoutMillis, PMF.get/setQueryTimeoutMillis. I'd draw the line at QueryInterruptedMillis though.
        Hide
        Andy Jefferson added a comment -

        Patch for Query API changes, JDOQueryTimeoutException, JDOQueryInterruptedException, and associated changes for PMF property.

        I've made the timeout value as milliseconds so we have sufficient granularity to cater for all eventualities. Anyone prefer seconds?

        Show
        Andy Jefferson added a comment - Patch for Query API changes, JDOQueryTimeoutException, JDOQueryInterruptedException, and associated changes for PMF property. I've made the timeout value as milliseconds so we have sufficient granularity to cater for all eventualities. Anyone prefer seconds?

          People

          • Assignee:
            Michael Bouschen
            Reporter:
            Andy Jefferson
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development