timoninmaxim commented on code in PR #12394:
URL: https://github.com/apache/ignite/pull/12394#discussion_r2619080427
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheDistributedQueryManager.java:
##########
@@ -227,6 +227,16 @@ protected void removeQueryFuture(long reqId) {
threads.remove(req.id());
}
}
+ } else {
+ GridCacheQueryResponse closedResponse = new
GridCacheQueryResponse(
+ cctx.cacheId(),
+ req.id(),
+ new NoSuchElementException("Iterator has been
closed."),
Review Comment:
Let's throw QueryCancelledException instead
##########
modules/core/src/main/java/org/apache/ignite/internal/QueryMXBeanImpl.java:
##########
@@ -173,7 +176,18 @@ private static class CancelScan implements
IgniteClosure<T3<UUID, String, Long>,
return null;
}
- ctx.queries().removeQueryResult(arg.get1(), arg.get3());
+ GridCacheQueryManager qryMgr = ctx.queries();
+
+ if (qryMgr instanceof GridCacheDistributedQueryManager) {
Review Comment:
it's always true
##########
modules/indexing/src/test/java/org/apache/ignite/util/KillCommandsTests.java:
##########
@@ -161,21 +157,30 @@ public static void checkScanQueryCancelBeforeFetching(
// Cancel first query.
qryCanceler.accept(qryInfo);
+ // Checking that second query works fine after canceling first.
+ for (int i = 0; i < PAGE_SZ * PAGES_CNT - 1; i++)
+ assertNotNull(iter2.next());
+
+ qry2.close();
+
// Fetch of the next page should throw the exception. New page is
delivered in parallel to iterating.
- assertThrowsWithCause(() -> {
+ try {
for (int i = 0; i < PAGE_SZ * PAGES_CNT - 1; i++)
assertNotNull(iter1.next());
- return null;
- }, IgniteCheckedException.class);
+ fail("Expected IgniteCheckedException but no exception was
thrown");
+ }
+ catch (Exception e) {
+ Throwable cause = e instanceof RuntimeException && e.getCause() !=
null ? e.getCause() : e;
- // Checking that second query works fine after canceling first.
- for (int i = 0; i < PAGE_SZ * PAGES_CNT - 1; i++)
- assertNotNull(iter2.next());
+ if (cause instanceof IgniteCheckedException &&
+ cause.getMessage() != null &&
+ cause.getMessage().contains("Received next page request
after iterator was removed")) {
+ fail("Test failed: caught unexpected IgniteCheckedException: "
+ cause.getMessage());
+ }
+ }
Review Comment:
Let's also add a check that GridCacheQueryManager.RequestFutureMap is empty
on all nodes after query cancellation.
##########
modules/indexing/src/test/java/org/apache/ignite/util/KillCommandsTests.java:
##########
@@ -161,21 +157,30 @@ public static void checkScanQueryCancelBeforeFetching(
// Cancel first query.
qryCanceler.accept(qryInfo);
+ // Checking that second query works fine after canceling first.
+ for (int i = 0; i < PAGE_SZ * PAGES_CNT - 1; i++)
+ assertNotNull(iter2.next());
+
+ qry2.close();
+
// Fetch of the next page should throw the exception. New page is
delivered in parallel to iterating.
- assertThrowsWithCause(() -> {
+ try {
for (int i = 0; i < PAGE_SZ * PAGES_CNT - 1; i++)
assertNotNull(iter1.next());
- return null;
- }, IgniteCheckedException.class);
+ fail("Expected IgniteCheckedException but no exception was
thrown");
+ }
+ catch (Exception e) {
+ Throwable cause = e instanceof RuntimeException && e.getCause() !=
null ? e.getCause() : e;
- // Checking that second query works fine after canceling first.
Review Comment:
Pls, revert changes for `qry2` iteration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
