Copilot commented on code in PR #7134:
URL: https://github.com/apache/kyuubi/pull/7134#discussion_r2209750057
##########
kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/KyuubiStatementTest.java:
##########
@@ -54,4 +57,49 @@ public void testaddBatch() throws SQLException {
stmt.addBatch(null);
}
}
+
+ @Test
+ public void testThrowKyuubiSQLExceptionWhenExecuteSqlOnClosedStmt()
+ throws SQLException, InterruptedException {
+ try (KyuubiStatement stmt = new KyuubiStatement(null, null, null)) {
+ AtomicReference<Throwable> assertionFailure = new AtomicReference<>();
+ CountDownLatch latch = new CountDownLatch(1);
+
+ Thread thread1 =
+ new Thread(
+ () -> {
+ try {
+ latch.countDown(); // 通知 thread2 可以开始了
Review Comment:
[nitpick] Test comments are written in Chinese; for consistency and clarity
in the codebase, consider using English comments.
```suggestion
latch.countDown(); // Notify thread2 that it can start
```
##########
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 {
Review Comment:
[nitpick] The lock in cancel() encompasses the network call to
`CancelOperation`, which may block other threads; consider limiting the locked
section to only the critical access to `stmtHandle` and `isCancelled` to reduce
contention.
```suggestion
public void cancel() throws SQLException {
TCancelOperationReq cancelReq = null;
```
##########
kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/KyuubiStatementTest.java:
##########
@@ -54,4 +57,49 @@ public void testaddBatch() throws SQLException {
stmt.addBatch(null);
}
}
+
+ @Test
+ public void testThrowKyuubiSQLExceptionWhenExecuteSqlOnClosedStmt()
+ throws SQLException, InterruptedException {
+ try (KyuubiStatement stmt = new KyuubiStatement(null, null, null)) {
+ AtomicReference<Throwable> assertionFailure = new AtomicReference<>();
+ CountDownLatch latch = new CountDownLatch(1);
+
+ Thread thread1 =
+ new Thread(
+ () -> {
+ try {
+ latch.countDown(); // 通知 thread2 可以开始了
+ stmt.close();
+ } catch (SQLException e) {
+ assertionFailure.set(e);
+ }
+ });
+
+ Thread thread2 =
+ new Thread(
+ () -> {
+ try {
+ latch.await();
+ KyuubiSQLException ex =
+ assertThrows(KyuubiSQLException.class, () ->
stmt.execute("SELECT 1"));
+ assertEquals("Can't execute after statement has been
closed", ex.getMessage());
+ } catch (Throwable t) {
+ // 捕获所有异常，包括断言失败
Review Comment:
[nitpick] Test comments are written in Chinese; for consistency and clarity
in the codebase, consider using English comments.
```suggestion
// Catch all exceptions, including assertion failures
```
##########
kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/KyuubiStatementTest.java:
##########
@@ -54,4 +57,49 @@ public void testaddBatch() throws SQLException {
stmt.addBatch(null);
}
}
+
+ @Test
+ public void testThrowKyuubiSQLExceptionWhenExecuteSqlOnClosedStmt()
+ throws SQLException, InterruptedException {
+ try (KyuubiStatement stmt = new KyuubiStatement(null, null, null)) {
+ AtomicReference<Throwable> assertionFailure = new AtomicReference<>();
+ CountDownLatch latch = new CountDownLatch(1);
+
+ Thread thread1 =
+ new Thread(
+ () -> {
+ try {
+ latch.countDown(); // 通知 thread2 可以开始了
+ stmt.close();
Review Comment:
The latch is released before stmt.close() completes, which may lead to
intermittent test failures; consider counting down the latch after close or
using separate synchronization points to enforce ordering.
```suggestion
stmt.close();
latch.countDown(); // 通知 thread2 可以开始了
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
