You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "korot-7 (via GitHub)" <gi...@apache.org> on 2025/10/03 11:11:33 UTC
[PR] Kill Scan [ignite]
korot-7 opened a new pull request, #12394:
URL: https://github.com/apache/ignite/pull/12394
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-15365 Fix "KILL SCAN" to cancel queries on remote nodes [ignite]
Posted by "timoninmaxim (via GitHub)" <gi...@apache.org>.
timoninmaxim commented on code in PR #12394:
URL: https://github.com/apache/ignite/pull/12394#discussion_r2650590351
##########
modules/indexing/src/test/java/org/apache/ignite/util/KillCommandsTests.java:
##########
@@ -161,13 +162,24 @@ public static void checkScanQueryCancelBeforeFetching(
// Cancel first query.
qryCanceler.accept(qryInfo);
+ checkScanQueryResources(cli, srvs, qryInfo.get3());
+
// Fetch of the next page should throw the exception. New page is delivered in parallel to iterating.
- assertThrowsWithCause(() -> {
+ try {
Review Comment:
GridTestUtils.assertThrows accepts "message" param.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Kill Scan [ignite]
Posted by "timoninmaxim (via GitHub)" <gi...@apache.org>.
timoninmaxim commented on code in PR #12394:
URL: https://github.com/apache/ignite/pull/12394#discussion_r2622301136
##########
modules/indexing/src/test/java/org/apache/ignite/util/KillCommandsTests.java:
##########
@@ -176,6 +187,8 @@ public static void checkScanQueryCancelBeforeFetching(
checkScanQueryResources(cli, srvs, qryInfo.get3());
qry2.close();
+
+ checkRequestFutureMapEmpty(cli, srvs, qryInfo.get1());
Review Comment:
Can we check it after `qryCanceler.accept(qryInfo)` and validate that this query is cleared immediately after cancelling?
##########
modules/indexing/src/test/java/org/apache/ignite/util/KillCommandsTests.java:
##########
@@ -254,6 +267,40 @@ private static void checkScanQueryResources(IgniteEx cli, List<IgniteEx> srvs, l
}
}
+ /**
+ * Checks that RequestFutureMap is empty on all nodes after query cancellation.
+ *
+ * @param cli Client node.
+ * @param srvs Server nodes.
+ * @param originNodeId Origin node ID.
+ */
+ private static void checkRequestFutureMapEmpty(
+ IgniteEx cli,
+ List<IgniteEx> srvs,
+ UUID originNodeId
+ ) {
+ List<IgniteEx> allNodes = new ArrayList<>(srvs);
+ allNodes.add(cli);
+
+ for (IgniteEx node : allNodes) {
+ int cacheId = CU.cacheId(DEFAULT_CACHE_NAME);
+ GridCacheContext<?, ?> ctx = node.context().cache().context().cacheContext(cacheId);
+
+ if (ctx == null) {
+ continue;
Review Comment:
Actually this happens for `cli` node, then you don't need add it to check.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheDistributedQueryManager.java:
##########
@@ -228,6 +229,17 @@ protected void removeQueryFuture(long reqId) {
}
}
}
+ else {
+ GridCacheQueryResponse res = new GridCacheQueryResponse(
+ cctx.cacheId(),
+ req.id(),
+ new QueryCancelledException("Iterator has been closed."),
+ cctx.deploymentEnabled()
+ );
+
+ sendQueryResponse(sndId, res, 0);
+
Review Comment:
remove this NL
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheDistributedQueryManager.java:
##########
@@ -228,6 +229,17 @@ protected void removeQueryFuture(long reqId) {
}
}
}
+ else {
+ GridCacheQueryResponse res = new GridCacheQueryResponse(
+ cctx.cacheId(),
+ req.id(),
+ new QueryCancelledException("Iterator has been closed."),
Review Comment:
Query has been cancelled
##########
modules/core/src/main/java/org/apache/ignite/internal/QueryMXBeanImpl.java:
##########
@@ -173,7 +175,10 @@ private static class CancelScan implements IgniteClosure<T3<UUID, String, Long>,
return null;
}
- ctx.queries().removeQueryResult(arg.get1(), arg.get3());
+ GridCacheQueryRequest cancelReq = GridCacheQueryRequest.cancelRequest(ctx, arg.get3(), false);
+
+ GridCacheDistributedQueryManager distQryMgr = (GridCacheDistributedQueryManager)ctx.queries();
+ distQryMgr.processQueryRequest(arg.get1(), cancelReq);
Review Comment:
You can inline all vars as all of them are used only once.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-15365 Fix "KILL SCAN" to cancel queries on remote nodes [ignite]
Posted by "timoninmaxim (via GitHub)" <gi...@apache.org>.
timoninmaxim commented on code in PR #12394:
URL: https://github.com/apache/ignite/pull/12394#discussion_r2650583394
##########
modules/core/src/main/java/org/apache/ignite/internal/QueryMXBeanImpl.java:
##########
@@ -173,7 +175,9 @@ private static class CancelScan implements IgniteClosure<T3<UUID, String, Long>,
return null;
}
- ctx.queries().removeQueryResult(arg.get1(), arg.get3());
+ ((GridCacheDistributedQueryManager)ctx.queries()).
Review Comment:
A dot should be first symbol in a line.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Kill Scan [ignite]
Posted by "timoninmaxim (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org