Hi Andy, hi Craig,
I'm surprised about the query execution time of 60 ms to 2 ms. Andy,
your hardware seems to be much faster than mine :-).
I agree we should change the test such that the query takes longer to
execute. I propose to create more test instances, then the join (as part
of the query) becomes more expensive.
Attached you find a patch with the following changes of the test class:
- it creates 5000 PCPoint and PCPoint2 instances (instead of 1000)
- uses a barrier to synchronize the threads
- compiles the query before staring the thread
Regards Michael
Hi Andy,
On Mar 13, 2010, at 3:29 AM, Andy Jefferson wrote:
Hi Michael,
I added a Thread.sleep(1000) to the main thread in order to give the
other thread a chance to start the query before the main thread
cancels
it. Could you please give it a try?
Giving something a second to get started seems excessive, I know
Derby is slow
but ... ;-)
I tried with a value more like 80 or 100 (millis) and managed **on
occasions**
to get a JDOQueryInterruptedException (and pass the test). Other
times the
cancel still got in first. I also added a query.compile() before
starting the
threads hence less for the query execute to do before starting, but
still
intermittent. The intermittency will also be affected by the machine
being
used, hence the number is arbitrary. The query execution time (in the
datastore) is anywhere between 60ms and 2ms on my machine.
This test will always be somewhat sensitive to timing, but we should
be able to minimize the timing effects.
The query itself should be long enough to allow the cancel to have an
effect. The query execution time of 60 ms to 2 ms. [sic] doesn't sound
long enough. Maybe we need to make the query more complex. But still,
as machines get faster the query execution times will get smaller.
The other thing we discussed earlier was the possibility of needing a
barrier so we could guarantee that the two threads were synchronized
at some point. The main thread calling start doesn't quite guarantee
synchronization, while a barrier of size two should do it. Still,
assuming that the query is compiled before the barrier, and the main
thread yields to the query thread (sleep should do this just fine) we
should be able to get the query to start and the cancel to interrupt it.
Depends what the cancel is allowing cancel over; to be consistent
with the
timeout I assume it is cancel the datastore operation (as opposed to
the whole
query execute process ... compile, generate PreparedStatement,
populate
parameter values, execute in the datastore, return results), and so
DataNucleus only starts the execution in a separate thread and
maintains the
handle on that thread for cancellation purposes. If this execution
thread is
either not yet created, or is now finished then the test will fail,
always.
Another good high level discussion. Since we have the datastore read
timeout, the purpose of the cancel is to stop query execution before
the timeout expires. Since we assume that the main thread and the
query thread have ways of communicating as long as the query thread is
not stuck in a datastore wait, we are only concerned about getting the
query thread unstuck.
So if the query has already returned results, the purpose for cancel
doesn't exist. There still may be a timing issue in real life where
the main thread sees that the query thread is stuck, but by the time
it gets around to canceling the query, the query thread is no longer
stuck. So it doesn't really matter whether the cancel worked or not.
That's why I agree that a reasonable behavior is having the cancel be
a no-op in case it can't find anything to do.
But we still want a test case. Do we need the test case to perform
some timing tests to see how long queries take and adjust the wait
time accordingly? Do we want to try different values and require that
at least one value succeed? Start with a value of 1 ms and raise by
increments until the cancel works?
Shouldn't the test also take into account if the query does actually
return
the results before the cancel can be called ?
This could be part of the heuristics as noted above. Much better if we
can figure out a good barrier/sleep strategy that at least works for
now.
Craig
Regards
--
Andy
DataNucleus (http://www.datanucleus.org)
Craig L Russell
Architect, Oracle
http://db.apache.org/jdo
408 276-5638 mailto:[email protected]
P.S. A good JDO? O, Gasp!
--
*Michael Bouschen*
*Prokurist*
akquinet t...@spree GmbH
Bülowstr. 66, D-10783 Berlin
Fon: +49 30 235 520-33
Fax: +49 30 217 520-12
Email: [email protected]
Url: www.akquinet.de <http://www.akquinet.de>
akquinet t...@spree GmbH, Berlin
Geschäftsführung: Martin Weber, Prof. Dr. Christian Roth
Amtsgericht Berlin-Charlottenburg HRB 86780 B
USt.-Id. Nr.: DE 225 964 680
Index: src/java/org/apache/jdo/tck/query/api/QueryCancel.java
===================================================================
--- src/java/org/apache/jdo/tck/query/api/QueryCancel.java (Revision
922445)
+++ src/java/org/apache/jdo/tck/query/api/QueryCancel.java (Arbeitskopie)
@@ -17,6 +17,8 @@
package org.apache.jdo.tck.query.api;
+import java.util.concurrent.CyclicBarrier;
+
import junit.framework.AssertionFailedError;
import javax.jdo.JDOFatalException;
@@ -50,8 +52,11 @@
public class QueryCancel extends QueryTest {
/** Time for the main thread to sleep after starting a parallel thread. */
- private static int MAIN_SLEEP_MILLIS = 1000;
+ private static int MAIN_SLEEP_MILLIS = 20;
+ /** Number of instances to be created. */
+ private static int NO_OF_INSTANCES = 5000;
+
/** */
private static final String ASSERTION_FAILED =
"Assertion A14.6.1-8 (QueryCancel) failed: ";
@@ -77,18 +82,24 @@
/** */
public void testCancel() throws Exception {
PersistenceManager pm = getPM();
+ // Test query
Query query = pm.newQuery(SSJDOQL);
+ query.compile();
// Thread executing the query
+ CyclicBarrier barrier = new CyclicBarrier(2);
ThreadExceptionHandler group = new ThreadExceptionHandler();
- QueryExecutor runnable = new QueryExecutor(pm, query);
+ QueryExecutor runnable = new QueryExecutor(pm, query, barrier);
Thread t = new Thread(group, runnable, "Query Executor");
t.start();
- // Wait for a second such that the other thread can execute the query
- Thread.sleep(MAIN_SLEEP_MILLIS);
+ try {
+ // Wait for the other thread
+ barrier.await();
- try {
+ // Wait a couple of millis such that the other thread can start
query execution
+ Thread.sleep(MAIN_SLEEP_MILLIS);
+
// cancel query
query.cancel(t);
if (!isQueryCancelSupported()) {
@@ -119,19 +130,25 @@
/** */
public void testCancelAll() throws Exception {
PersistenceManager pm = getPM();
+ // Test query
Query query = pm.newQuery(SSJDOQL);
+ query.compile();
// Thread executing the query
+ CyclicBarrier barrier = new CyclicBarrier(2);
ThreadExceptionHandler group = new ThreadExceptionHandler();
- QueryExecutor runnable = new QueryExecutor(pm, query);
+ QueryExecutor runnable = new QueryExecutor(pm, query, barrier);
Thread t = new Thread(group, runnable, "Query Executor");
t.start();
- // Wait for a second such that the other thread can execute the query
- Thread.sleep(MAIN_SLEEP_MILLIS);
-
try {
// cancel query
+ // Wait for the other thread
+ barrier.await();
+
+ // Wait a couple of millis such that the other thread can start
query execution
+ Thread.sleep(MAIN_SLEEP_MILLIS);
+
query.cancelAll();
if (!isQueryCancelSupported()) {
fail(ASSERTION_FAILED,
@@ -162,17 +179,23 @@
class QueryExecutor implements Runnable {
PersistenceManager pm;
+ CyclicBarrier barrier;
Query query;
- QueryExecutor(PersistenceManager pm, Query query) {
+ QueryExecutor(PersistenceManager pm, Query query, CyclicBarrier
barrier) {
this.pm = pm;
this.query = query;
+ this.barrier = barrier;
}
public void run() {
Transaction tx = pm.currentTransaction();
try {
tx.begin();
+
+ // wait for the other thread
+ barrier.await();
+
Object result = query.execute();
tx.commit();
tx = null;
@@ -189,6 +212,9 @@
"if query canceling is not supported.");
}
}
+ catch (Exception ex) {
+ throw new RuntimeException(ex);
+ }
finally {
if ((tx != null) && tx.isActive())
tx.rollback();
@@ -208,11 +234,11 @@
Transaction tx = pm.currentTransaction();
try {
tx.begin();
- for (int i = 0; i < 1000; i++) {
+ for (int i = 0; i < NO_OF_INSTANCES; i++) {
PCPoint obj = new PCPoint(i, i);
pm.makePersistent(obj);
}
- for (int i = 0; i < 1000; i++) {
+ for (int i = 0; i < NO_OF_INSTANCES; i++) {
PCPoint2 obj = new PCPoint2(i, i);
pm.makePersistent(obj);
}
