Copilot commented on code in PR #7134:
URL: https://github.com/apache/kyuubi/pull/7134#discussion_r2207586723
##########
kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/KyuubiStatementTest.java:
##########
@@ -54,4 +55,36 @@ public void testaddBatch() throws SQLException {
stmt.addBatch(null);
}
}
+
+ @Test
+ public void testThrowKyuubiSQLExceptionWhenExecuteSqlOnClosedStmt()
+ throws SQLException, InterruptedException {
+ KyuubiStatement stmt = new KyuubiStatement(null, null, null);
+ try {
+ Thread thread1 =
+ new Thread(
+ () -> {
+ try {
+ stmt.close();
+ } catch (SQLException e) {
+ throw new RuntimeException(e);
+ }
+ });
+ Thread thread2 =
+ new Thread(
+ () -> {
+ assertEquals(
+ "Can't execute after statement has been closed",
+ assertThrows(KyuubiSQLException.class, () ->
stmt.execute("SELECT 1"))
+ .getMessage());
Review Comment:
This test may be flaky because `thread1` and `thread2` start concurrently
without ordering, so the close might not happen before execute. Consider using
a `CountDownLatch` or starting `thread2` after `thread1.join()` to guarantee
the intended order.
```suggestion
try {
latch.await();
assertEquals(
"Can't execute after statement has been closed",
assertThrows(KyuubiSQLException.class, () ->
stmt.execute("SELECT 1"))
.getMessage());
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
```
##########
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 {
+ stmtHandleAccessLock.lock();
+ checkConnection("cancel");
+ if (isCancelled) {
+ return;
+ }
if (stmtHandle != null) {
TCancelOperationReq cancelReq = new TCancelOperationReq(stmtHandle);
TCancelOperationResp cancelResp = client.CancelOperation(cancelReq);
Utils.verifySuccessWithInfo(cancelResp.getStatus());
}
+ isCancelled = true;
} catch (SQLException e) {
throw e;
Review Comment:
[nitpick] The `catch (SQLException e)` block merely rethrows the exception
without adding context. It can be removed to simplify the code and rely on the
`finally` block for unlocking.
```suggestion
```
--
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
