-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix flaky testFailStuckSplitTasks unit test #13441
Conversation
core/trino-main/src/test/java/io/trino/execution/TestSqlTaskManager.java
Outdated
Show resolved
Hide resolved
913a3bd
to
d233b92
Compare
core/trino-main/src/test/java/io/trino/execution/TestSqlTaskManager.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/execution/TestSqlTaskManager.java
Outdated
Show resolved
Hide resolved
c801efc
to
97f32a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
core/trino-main/src/test/java/io/trino/execution/TestSqlTaskManager.java
Outdated
Show resolved
Hide resolved
bf7f754
to
62bc7ff
Compare
core/trino-main/src/test/java/io/trino/execution/TestSqlTaskManager.java
Outdated
Show resolved
Hide resolved
@@ -264,6 +265,10 @@ public void testFailStuckSplitTasks() | |||
// Here we explicitly enqueue an indefinite running split runner | |||
taskExecutor.enqueueSplits(taskHandle, false, ImmutableList.of(mockSplitRunner)); | |||
taskExecutor.start(); | |||
// wait for the task executor to start processing the split | |||
while (!mockSplitRunner.isStarted()) { | |||
Thread.sleep(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I assume it might be reasonable to have another future that would identify a split finished execution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The split is never finished // interrupted. It is only terminated at taskExecutor.stop().
The unit test only verified that the sqlTaskManager could identify the task with a long-running split and mark the task to the failed state.
It is because the sqlTaskManager issues failTask() for the task, which eventually issues a list of callback functions while doing the state transition of the task. In a real Trino cluster, one of which is responsible for interrupting the DriverSplitRunner(an implementation of SplitRunner), however, in the unit test, there is no such callback function. And it is another unit test that checks when doing state changes, the registered callback functions can be called successfully.
Let me know if you have a suggestion for improvement!
1c267b8
to
7cf70e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % comments
core/trino-main/src/test/java/io/trino/execution/TestSqlTaskManager.java
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/execution/TestSqlTaskManager.java
Outdated
Show resolved
Hide resolved
7cf70e0
to
1a65363
Compare
Description
Fix flaky testFailStuckSplitTasks unit test.
There is a race condition between unit test thread and the task executor thread. Added checking via settableFuture in the unit test thread to ensure task executor to process splits first.
Fix
Query Engine
Fix flaky testFailStuckSplitTasks unit test.
Related issues, pull requests, and links
#12392
#13437
Documentation
(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
(x) No release notes entries required.
( ) Release notes entries required with the following suggested text: