[KYUUBI #5392] Add query timeout monitor on server-side in ExecuteStatement #5398
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5398 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 588 588
Lines 33454 33465 +11
Branches 4401 4401
======================================
- Misses 33454 33465 +11
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Have a check on this draft implementation for server-side timeout monitoring. |
| } | ||
|
|
||
| object ExecuteStatement { | ||
| private[kyuubi] lazy val timeoutCheckPool: ScheduledExecutorService = |
There was a problem hiding this comment.
Reusing a SingleThreadScheduledExecutor for all operations may cause scheduling delays.
There was a problem hiding this comment.
the scheduled task is quite light, compared with a bunch of monitor threads, is such a delay matter?
There was a problem hiding this comment.
Not a big deal, as the thread will be shutdown immediately once query is finished
There was a problem hiding this comment.
Change to use the addTimeoutMonitor method, which starts as a single-thread executor pool and seems never closed. It seems never be closed and it's opened for each ExecuteStatement operation.
Does it cause the thread leaks? Or let's force the timeout check in a dedicated global ForkJoinPool ?
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/ExecuteStatement.scala
Outdated
Show resolved
Hide resolved
bowenliang123
force-pushed
the
stmt-timeout
branch
2 times, most recently
from
75468b7 to
5913698
Compare
bowenliang123
changed the title
bowenliang123
changed the title
pan3793
changed the title
bowenliang123
force-pushed
the
stmt-timeout
branch
from
4513671 to
e8dd3a0
Compare
bowenliang123
force-pushed
the
stmt-timeout
branch
from
d460192 to
72479a7
Compare
| val asyncOperation: Runnable = () => | ||
| try { | ||
| if (isTimeoutMonitorEnabled) { | ||
| addTimeoutMonitor(queryTimeout) |
There was a problem hiding this comment.
how about moving it before executeStatement() as we did on the engine side?
There was a problem hiding this comment.
It's done on purpose for considering and tolerating the time for connecting and submitting statement to the engine.
But let's move it before executeStatement this time for semantic consistency.
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/ExecuteStatement.scala
Outdated
Show resolved
Hide resolved
bowenliang123
force-pushed
the
stmt-timeout
branch
from
58f753c to
f5733b3
Compare
…tement ### _Why are the changes needed?_ As reported in #5392, currently the server is unable to guarantee that the statement timed-out when the engine may have no proper response for the server's request therefore the query timeout does not work. Introduce a server-side statement query timeout monitor, to ensure the time-out query statements are set to TIMEOUT state and help the JDBC client get out of the blocked status. ### _How was this patch tested?_ - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request ### _Was this patch authored or co-authored using generative AI tooling?_ No. Closes #5398 from bowenliang123/stmt-timeout. Closes #5392 f5733b3 [Bowen Liang] use addTimeoutMonitor for server-side query timeout checks Authored-by: Bowen Liang <[email protected]> Signed-off-by: liangbowen <[email protected]> (cherry picked from commit bdc28ac) Signed-off-by: liangbowen <[email protected]>
|
Thanks, merged to master(1.9.0) and branch-1.8 (1.8.0). |
Why are the changes needed?
As reported in #5392, currently the server is unable to guarantee that the statement timed-out when the engine may have no proper response for the server's request therefore the query timeout does not work.
Introduce a server-side statement query timeout monitor, to ensure the time-out query statements are set to TIMEOUT state and help the JDBC client get out of the blocked status.
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request
Was this patch authored or co-authored using generative AI tooling?
No.