korlov42 commented on code in PR #4907:
URL: https://github.com/apache/ignite-3/pull/4907#discussion_r1889797052
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/fsm/CursorInitializationPhaseHandler.java:
##########
@@ -54,11 +55,9 @@ public Result handle(Query query) {
query.cursor = cursor;
- query.cancel.add(timeout -> {
- if (!timeout) {
- cursor.closeAsync(true);
- }
- });
+ query.cancel.add(timeout -> dataCursor.cancelAsync(
+ timeout ? CancellationReason.TIMEOUT :
CancellationReason.CANCEL
Review Comment:
> can we also remove the code that adds timeout handling in executeDdl and
executeKill methods of ExecutionServiceImpl?
Unfortunately, this is not only about removal from these two methods. We
also need to remove according tests from `ExecutionServiceImplTest` (which is
ok) and write another somewhere replacing old ones (since we would like to rely
on cancellation code in one of the ExecutionPhase, new tests must involve
`TestCluster`). I believe this is out of the scope of current issue. Besides,
we already has
[IGNITE-23792](https://issues.apache.org/jira/browse/IGNITE-23792), thus I
would prefer to don't touch this part in my PR, If you don't mind.
> сan we add a test that will reproduce (constantly) the issue that this
patch solves?
tbh, I have no idea how to make it to _steadily_ reproduces this race.
During investigation, I just added `Thread.sleep(2)` before adding cancellation
handling for cursor, but this is not acceptable to have `sleep()` in the
production code for this. To make sure (kinda) that problem is gone, I've run
JDBC job on TC 15 times
([proof](https://ci.ignite.apache.org/buildConfiguration/ApacheIgnite3xGradle_Test_IntegrationTests_ModuleJdbc?branch=pull%2F4907#all-projects))
and got no failures. For my previous patch tests had failed after ~5th
iterations.
--
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]
