You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "ruanwenjun (via GitHub)" <gi...@apache.org> on 2025/11/17 13:51:26 UTC
[PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
ruanwenjun opened a new pull request, #7249:
URL: https://github.com/apache/kyuubi/pull/7249
### Why are the changes needed?
close #7248
- Cancel the jdbc statement when receive a kyuubi cancel operation
### How was this patch tested?
Test by new ut case
### Was this patch authored or co-authored using generative AI tooling?
No
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544332946
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/starrocks/StarRocksOperationWithEngineSuite.scala:
##########
@@ -75,4 +75,26 @@ class StarRocksOperationWithEngineSuite extends StarRocksOperationSuite with Hiv
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("starrocks - JDBC ExecuteStatement operation cancel should kill SQL statement") {
+ withSessionHandle { (client, handle) =>
+ val tExecuteStatementReq = new TExecuteStatementReq()
+ tExecuteStatementReq.setSessionHandle(handle)
+ tExecuteStatementReq.setStatement("SELECT sleep(1200)")
+ tExecuteStatementReq.setRunAsync(true)
+ val tExecuteStatementResp = client.ExecuteStatement(tExecuteStatementReq)
+
+ // todo:we need to improvement the cancel mechanism in starrocks server side,
+ // once a query submitted, we should be able to cancel it immediately.
Review Comment:
ah, that makes sense
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544558269
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -120,12 +120,31 @@ class ExecuteStatement(
super.validateFetchOrientation(order)
}
+ override def cancel(): Unit = withLockRequired {
+ if (!isTerminalState(state)) {
+ setState(OperationState.CANCELED)
+ // TODO: If `shouldRunAsync` is true, the statement is initialized lazily.
+ // When a SQL is submitted and immediately canceled, `jdbcStatement` may still be null,
+ // which can lead to the cancellation not taking effect.
+ if (jdbcStatement != null) {
+ dialect.cancelStatement(jdbcStatement)
+ jdbcStatement = null
+ } else {
+ warn(s"The cancel operation $statementId might be ignore due to jdbcStatement is null.")
Review Comment:
```suggestion
warn(s"Ignore cancel operation $statementId due to jdbcStatement is null.")
```
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544225911
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/starrocks/StarRocksOperationWithEngineSuite.scala:
##########
@@ -75,4 +75,26 @@ class StarRocksOperationWithEngineSuite extends StarRocksOperationSuite with Hiv
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("starrocks - JDBC ExecuteStatement operation cancel should kill SQL statement") {
+ withSessionHandle { (client, handle) =>
+ val tExecuteStatementReq = new TExecuteStatementReq()
+ tExecuteStatementReq.setSessionHandle(handle)
+ tExecuteStatementReq.setStatement("SELECT sleep(1200)")
+ tExecuteStatementReq.setRunAsync(true)
+ val tExecuteStatementResp = client.ExecuteStatement(tExecuteStatementReq)
+
+ // todo:we need to improvement the cancel mechanism in starrocks server side,
+ // once a query submitted, we should be able to cancel it immediately.
Review Comment:
Sorry, I didn't express myself clearly. I moved this `todo` to `ExecuteStatement`.
This is an issue present in all JDBC engine in kyuubi. When using asyn mode, the statement creation is delayed. If we cancel immediately after submitting the SQL, there is a chance the cancel operation might be ignored, if the statement has not yet been created at that point,
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544328416
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/mysql/OperationWithEngineSuite.scala:
##########
@@ -75,4 +78,26 @@ class OperationWithEngineSuite extends MySQLOperationSuite with HiveJDBCTestHelp
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("MySQL - JDBC ExecuteStatement cancel operation should kill SQL statement") {
+ failAfter(20.seconds) {
+ withSessionHandle { (client, handle) =>
+ val tExecuteStatementReq = new TExecuteStatementReq()
+ tExecuteStatementReq.setSessionHandle(handle)
+ // The SQL will sleep 120s
+ tExecuteStatementReq.setStatement("SELECT sleep(120)")
+ tExecuteStatementReq.setRunAsync(true)
+ val tExecuteStatementResp = client.ExecuteStatement(tExecuteStatementReq)
+ assert(tExecuteStatementResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
+
+ Thread.sleep(1000) // wait for statement to start executing
Review Comment:
I can call the detail, but do we have a reliable way to check that the stmt is running?
if so, we can write it like this
```
eventually(timeout(5.seconds), interval(100.millis)) {
// a reliable way to check op is executing
...
}
```
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/mysql/OperationWithEngineSuite.scala:
##########
@@ -75,4 +78,26 @@ class OperationWithEngineSuite extends MySQLOperationSuite with HiveJDBCTestHelp
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("MySQL - JDBC ExecuteStatement cancel operation should kill SQL statement") {
+ failAfter(20.seconds) {
+ withSessionHandle { (client, handle) =>
+ val tExecuteStatementReq = new TExecuteStatementReq()
+ tExecuteStatementReq.setSessionHandle(handle)
+ // The SQL will sleep 120s
+ tExecuteStatementReq.setStatement("SELECT sleep(120)")
+ tExecuteStatementReq.setRunAsync(true)
+ val tExecuteStatementResp = client.ExecuteStatement(tExecuteStatementReq)
+ assert(tExecuteStatementResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
+
+ Thread.sleep(1000) // wait for statement to start executing
Review Comment:
I can't call the detail, but do we have a reliable way to check that the stmt is running?
if so, we can write it like this
```
eventually(timeout(5.seconds), interval(100.millis)) {
// a reliable way to check op is executing
...
}
```
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544553712
##########
kyuubi-common/src/test/scala/org/apache/kyuubi/operation/HiveJDBCTestHelper.scala:
##########
@@ -124,11 +124,27 @@ trait HiveJDBCTestHelper extends JDBCTestHelper {
}
def waitForOperationToComplete(client: Iface, op: TOperationHandle): Unit = {
- val req = new TGetOperationStatusReq(op)
- var state = client.GetOperationStatus(req).getOperationState
- eventually(timeout(90.seconds), interval(100.milliseconds)) {
- state = client.GetOperationStatus(req).getOperationState
- assert(!Set(INITIALIZED_STATE, PENDING_STATE, RUNNING_STATE).contains(state))
+ assertOperationStatusIn(
+ client,
+ op,
+ Set(
+ FINISHED_STATE,
+ CANCELED_STATE,
+ CLOSED_STATE,
+ ERROR_STATE,
+ UKNOWN_STATE,
+ TIMEDOUT_STATE),
+ 90)
+ }
+
+ def assertOperationStatusIn(
+ client: Iface,
+ op: TOperationHandle,
+ status: Set[TOperationState],
+ timeoutInSeconds: Int): Unit = {
Review Comment:
it's actually `waitForOperationStatusIn`, and normally, we use millis for duration in most cases, so `timeoutInSeconds` => `timeoutMs`, and on the caller side, you can use named args `timeoutMs = 90000` to eliminate ambiguity
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544182560
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/starrocks/StarRocksOperationWithEngineSuite.scala:
##########
@@ -75,4 +75,26 @@ class StarRocksOperationWithEngineSuite extends StarRocksOperationSuite with Hiv
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("starrocks - JDBC ExecuteStatement operation cancel should kill SQL statement") {
Review Comment:
same here - StarRocks
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/mysql/OperationWithEngineSuite.scala:
##########
@@ -75,4 +75,23 @@ class OperationWithEngineSuite extends MySQLOperationSuite with HiveJDBCTestHelp
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("Mysql - JDBC ExecuteStatement operation cancel should kill SQL statement") {
Review Comment:
use the correct form - "MySQL", not "mySql", "Mysql", or something else
https://www.mysql.com/
operation cancel => cancel operation
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/mysql/OperationWithEngineSuite.scala:
##########
@@ -75,4 +75,23 @@ class OperationWithEngineSuite extends MySQLOperationSuite with HiveJDBCTestHelp
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("Mysql - JDBC ExecuteStatement operation cancel should kill SQL statement") {
+ withSessionHandle { (client, handle) =>
+ val tExecuteStatementReq = new TExecuteStatementReq()
+ tExecuteStatementReq.setSessionHandle(handle)
+ tExecuteStatementReq.setStatement("SELECT sleep(1200)")
Review Comment:
what's the unit? seconds? leave a comment here to avoid ambiguity, 1200s are too crazy, maybe 2min
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/starrocks/StarRocksOperationWithEngineSuite.scala:
##########
@@ -75,4 +75,26 @@ class StarRocksOperationWithEngineSuite extends StarRocksOperationSuite with Hiv
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("starrocks - JDBC ExecuteStatement operation cancel should kill SQL statement") {
+ withSessionHandle { (client, handle) =>
+ val tExecuteStatementReq = new TExecuteStatementReq()
+ tExecuteStatementReq.setSessionHandle(handle)
+ tExecuteStatementReq.setStatement("SELECT sleep(1200)")
+ tExecuteStatementReq.setRunAsync(true)
+ val tExecuteStatementResp = client.ExecuteStatement(tExecuteStatementReq)
+
+ // todo:we need to improvement the cancel mechanism in starrocks server side,
+ // once a query submitted, we should be able to cancel it immediately.
Review Comment:
it's unclear to me what the current behavior is from the words, and if you have StarRocks side issues/PR links, please leave them here.
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] JDBC engine should cancel the statement when receive cancel operation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #7249: [KYUUBI #7248] JDBC engine should cancel the statement when receive cancel operation
URL: https://github.com/apache/kyuubi/pull/7249
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2545014799
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/starrocks/WithStarRocksContainer.scala:
##########
@@ -26,7 +26,7 @@ import org.apache.kyuubi.engine.jdbc.WithJdbcServerContainer
trait WithStarRocksContainer extends WithJdbcServerContainer {
- private val starrocksDockerImage = "starrocks/allin1-ubuntu:3.1.6"
+ private val starrocksDockerImage = "starrocks/allin1-ubuntu:3.3.13"
Review Comment:
Upgrade the image since in `3.1.6` the sleep SQL might not be killed by be.
##########
kyuubi-common/src/test/scala/org/apache/kyuubi/operation/HiveJDBCTestHelper.scala:
##########
@@ -124,11 +124,27 @@ trait HiveJDBCTestHelper extends JDBCTestHelper {
}
def waitForOperationToComplete(client: Iface, op: TOperationHandle): Unit = {
- val req = new TGetOperationStatusReq(op)
- var state = client.GetOperationStatus(req).getOperationState
- eventually(timeout(90.seconds), interval(100.milliseconds)) {
- state = client.GetOperationStatus(req).getOperationState
- assert(!Set(INITIALIZED_STATE, PENDING_STATE, RUNNING_STATE).contains(state))
+ assertOperationStatusIn(
+ client,
+ op,
+ Set(
+ FINISHED_STATE,
+ CANCELED_STATE,
+ CLOSED_STATE,
+ ERROR_STATE,
+ UKNOWN_STATE,
+ TIMEDOUT_STATE),
+ 90)
+ }
+
+ def assertOperationStatusIn(
+ client: Iface,
+ op: TOperationHandle,
+ status: Set[TOperationState],
+ timeoutInSeconds: Int): Unit = {
Review Comment:
done
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2545069936
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/starrocks/StarRocksOperationWithEngineSuite.scala:
##########
@@ -75,4 +80,25 @@ class StarRocksOperationWithEngineSuite extends StarRocksOperationSuite with Hiv
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("starrocks - JDBC ExecuteStatement cancel operation should kill SQL statement") {
Review Comment:
```suggestion
test("StarRocks - JDBC ExecuteStatement cancel operation should kill SQL statement") {
```
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2545016159
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -120,12 +120,31 @@ class ExecuteStatement(
super.validateFetchOrientation(order)
}
+ override def cancel(): Unit = withLockRequired {
+ if (!isTerminalState(state)) {
+ setState(OperationState.CANCELED)
+ // TODO: If `shouldRunAsync` is true, the statement is initialized lazily.
+ // When a SQL is submitted and immediately canceled, `jdbcStatement` may still be null,
+ // which can lead to the cancellation not taking effect.
+ if (jdbcStatement != null) {
+ dialect.cancelStatement(jdbcStatement)
+ jdbcStatement = null
+ } else {
+ warn(s"The cancel operation $statementId might be ignore due to jdbcStatement is null.")
Review Comment:
done
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#issuecomment-3544911149
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/7249?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 0.00%. Comparing base ([`e7f06a3`](https://app.codecov.io/gh/apache/kyuubi/commit/e7f06a3fcb9db52e136a2928092697401ebaa695?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2e863b9`](https://app.codecov.io/gh/apache/kyuubi/commit/2e863b93acbdc52ff7584a29acd5d8f6c9d4c074?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #7249 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 696 696
Lines 43530 43530
Branches 5883 5883
======================================
Misses 43530 43530
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/kyuubi/pull/7249?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#issuecomment-3555426492
@pan3793 PTAL
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544225911
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/starrocks/StarRocksOperationWithEngineSuite.scala:
##########
@@ -75,4 +75,26 @@ class StarRocksOperationWithEngineSuite extends StarRocksOperationSuite with Hiv
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("starrocks - JDBC ExecuteStatement operation cancel should kill SQL statement") {
+ withSessionHandle { (client, handle) =>
+ val tExecuteStatementReq = new TExecuteStatementReq()
+ tExecuteStatementReq.setSessionHandle(handle)
+ tExecuteStatementReq.setStatement("SELECT sleep(1200)")
+ tExecuteStatementReq.setRunAsync(true)
+ val tExecuteStatementResp = client.ExecuteStatement(tExecuteStatementReq)
+
+ // todo:we need to improvement the cancel mechanism in starrocks server side,
+ // once a query submitted, we should be able to cancel it immediately.
Review Comment:
Sorry, I didn't express myself clearly. I moved this `todo` to `ExecuteStatement`. This is an issue present in all JDBC engine in kyuubi. When using asyn mode, the statement creation is delayed. If we cancel immediately after submitting the SQL, there is a chance the cancellation may fail. If the statement has not yet been created at that point,
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/starrocks/StarRocksOperationWithEngineSuite.scala:
##########
@@ -75,4 +75,26 @@ class StarRocksOperationWithEngineSuite extends StarRocksOperationSuite with Hiv
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("starrocks - JDBC ExecuteStatement operation cancel should kill SQL statement") {
+ withSessionHandle { (client, handle) =>
+ val tExecuteStatementReq = new TExecuteStatementReq()
+ tExecuteStatementReq.setSessionHandle(handle)
+ tExecuteStatementReq.setStatement("SELECT sleep(1200)")
+ tExecuteStatementReq.setRunAsync(true)
+ val tExecuteStatementResp = client.ExecuteStatement(tExecuteStatementReq)
+
+ // todo:we need to improvement the cancel mechanism in starrocks server side,
+ // once a query submitted, we should be able to cancel it immediately.
Review Comment:
Sorry, I didn't express myself clearly. I moved this `todo` to `ExecuteStatement`.
This is an issue present in all JDBC engine in kyuubi. When using asyn mode, the statement creation is delayed. If we cancel immediately after submitting the SQL, there is a chance the cancellation may fail. If the statement has not yet been created at that point,
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544201349
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/starrocks/StarRocksOperationWithEngineSuite.scala:
##########
@@ -75,4 +75,26 @@ class StarRocksOperationWithEngineSuite extends StarRocksOperationSuite with Hiv
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("starrocks - JDBC ExecuteStatement operation cancel should kill SQL statement") {
Review Comment:
Done
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#issuecomment-3543278274
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/7249?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 0.00%. Comparing base ([`e7f06a3`](https://app.codecov.io/gh/apache/kyuubi/commit/e7f06a3fcb9db52e136a2928092697401ebaa695?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ded4d77`](https://app.codecov.io/gh/apache/kyuubi/commit/ded4d776f2cee748d67e4a2f86fdfc583f799d70?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #7249 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 696 696
Lines 43530 43530
Branches 5883 5883
======================================
Misses 43530 43530
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/kyuubi/pull/7249?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544328416
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/mysql/OperationWithEngineSuite.scala:
##########
@@ -75,4 +78,26 @@ class OperationWithEngineSuite extends MySQLOperationSuite with HiveJDBCTestHelp
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("MySQL - JDBC ExecuteStatement cancel operation should kill SQL statement") {
+ failAfter(20.seconds) {
+ withSessionHandle { (client, handle) =>
+ val tExecuteStatementReq = new TExecuteStatementReq()
+ tExecuteStatementReq.setSessionHandle(handle)
+ // The SQL will sleep 120s
+ tExecuteStatementReq.setStatement("SELECT sleep(120)")
+ tExecuteStatementReq.setRunAsync(true)
+ val tExecuteStatementResp = client.ExecuteStatement(tExecuteStatementReq)
+ assert(tExecuteStatementResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
+
+ Thread.sleep(1000) // wait for statement to start executing
Review Comment:
I can't recall the detail, but do we have a reliable way to check that the stmt is running?
if so, we can write it like this
```
eventually(timeout(5.seconds), interval(100.millis)) {
// a reliable way to check op is executing
...
}
```
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544322519
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -120,12 +120,29 @@ class ExecuteStatement(
super.validateFetchOrientation(order)
}
+ override def cancel(): Unit = withLockRequired {
+ if (!isTerminalState(state)) {
+ setState(OperationState.CANCELED)
+ // TODO: If `shouldRunAsync` is true, the statement is initialized lazily.
+ // When a SQL is submitted and immediately canceled, `jdbcStatement` may still be null,
+ // which can lead to the cancellation not taking effect.
Review Comment:
let's add a warning message with opId for this case.
##########
externals/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/ExecuteStatement.scala:
##########
@@ -120,12 +120,29 @@ class ExecuteStatement(
super.validateFetchOrientation(order)
}
+ override def cancel(): Unit = withLockRequired {
+ if (!isTerminalState(state)) {
+ setState(OperationState.CANCELED)
+ // TODO: If `shouldRunAsync` is true, the statement is initialized lazily.
+ // When a SQL is submitted and immediately canceled, `jdbcStatement` may still be null,
+ // which can lead to the cancellation not taking effect.
Review Comment:
let's add a warning log with opId for this case.
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] JDBC engine should cancel the statement when receive cancel operation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#issuecomment-3557856444
JDBC engine tests have passed, merged to master/1.10, thanks @ruanwenjun
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7248] Ensure jdbc engine statements are canceled when receive cancel operation [kyuubi]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #7249:
URL: https://github.com/apache/kyuubi/pull/7249#discussion_r2544200972
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/mysql/OperationWithEngineSuite.scala:
##########
@@ -75,4 +75,23 @@ class OperationWithEngineSuite extends MySQLOperationSuite with HiveJDBCTestHelp
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("Mysql - JDBC ExecuteStatement operation cancel should kill SQL statement") {
+ withSessionHandle { (client, handle) =>
+ val tExecuteStatementReq = new TExecuteStatementReq()
+ tExecuteStatementReq.setSessionHandle(handle)
+ tExecuteStatementReq.setStatement("SELECT sleep(1200)")
Review Comment:
Done, add comment here.
##########
externals/kyuubi-jdbc-engine/src/test/scala/org/apache/kyuubi/engine/jdbc/mysql/OperationWithEngineSuite.scala:
##########
@@ -75,4 +75,23 @@ class OperationWithEngineSuite extends MySQLOperationSuite with HiveJDBCTestHelp
assert(tFetchResultsResp.getStatus.getStatusCode === TStatusCode.SUCCESS_STATUS)
}
}
+
+ test("Mysql - JDBC ExecuteStatement operation cancel should kill SQL statement") {
Review Comment:
Done
--
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@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org