Copilot commented on code in PR #7134:
URL: https://github.com/apache/kyuubi/pull/7134#discussion_r2203505535
##########
kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiStatement.java:
##########
@@ -124,23 +129,25 @@ public KyuubiStatement(
@Override
public void cancel() throws SQLException {
- checkConnection("cancel");
- if (isCancelled) {
- return;
- }
-
try {
+ checkConnection("cancel");
+ if (isCancelled) {
+ return;
+ }
+ stmtHandleChangeLock.lock();
Review Comment:
In `cancel()`, the method returns before acquiring the lock when
`isCancelled` is true, but the `finally` block still calls `unlock()`. This
will throw an `IllegalMonitorStateException`. Move
`stmtHandleChangeLock.lock()` before checking `isCancelled`, or restructure the
block so `unlock()` only runs when the lock is held.
##########
kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/KyuubiStatementTest.java:
##########
@@ -54,4 +58,29 @@ public void testaddBatch() throws SQLException {
stmt.addBatch(null);
}
}
+
+ @Test
+ public void testThrowKyuubiSQLExceptionWhenExecuteSqlOnClosedStmt() throws
SQLException {
+ KyuubiStatement stmt = new KyuubiStatement(null, null, null);
+ try {
+ ExecutorService executorService = Executors.newFixedThreadPool(2);
+ executorService.submit(
+ () -> {
+ try {
+ stmt.close();
+ } catch (SQLException e) {
+ throw new RuntimeException(e);
+ }
+ });
+ executorService.submit(
+ () -> {
+ Assert.assertEquals(
+ "Can't exectue after statement has been closed",
Review Comment:
There's a typo in the error message: "exectue" should be "execute". Please
correct the spelling in both the code that throws this message and the test
assertion.
```suggestion
"Can't execute after statement has been closed",
```
##########
kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/KyuubiStatementTest.java:
##########
@@ -54,4 +58,29 @@ public void testaddBatch() throws SQLException {
stmt.addBatch(null);
}
}
+
+ @Test
+ public void testThrowKyuubiSQLExceptionWhenExecuteSqlOnClosedStmt() throws
SQLException {
+ KyuubiStatement stmt = new KyuubiStatement(null, null, null);
+ try {
+ ExecutorService executorService = Executors.newFixedThreadPool(2);
Review Comment:
The test submits tasks to an `ExecutorService` but never shuts it down or
waits for task completion. This can lead to resource leaks and flaky test
behavior. Consider capturing the returned `Future`s (or using a
`CountDownLatch`), calling `shutdown()`, and `awaitTermination()` to ensure
tasks finish and propagate exceptions back to the test thread.
--
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-unsubscr...@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@kyuubi.apache.org
For additional commands, e-mail: notifications-h...@kyuubi.apache.org
