Skip to content

Commit f6524e4

Browse files
fix: Address Copilot review comments
- Make server availability probe static/one-time in ResetTest (avoids redundant client+query per test method) - Use CopyOnWriteArrayList in concurrent reset+query test for thread safety - Add closed guard in queryWithParams() for consistent IllegalStateException instead of confusing NPE after close()
1 parent 6f55f6c commit f6524e4

2 files changed

Lines changed: 21 additions & 9 deletions

File tree

src/main/java/ai/spice/SpiceClient.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,9 @@ public ArrowReader queryWithParams(String sql, Object... params) throws Executio
509509
if (Strings.isNullOrEmpty(sql)) {
510510
throw new IllegalArgumentException("No SQL query provided");
511511
}
512+
if (closed) {
513+
throw new IllegalStateException("Cannot query with a closed SpiceClient");
514+
}
512515

513516
logger.debug("Executing parameterized query with {} parameters: {}", params != null ? params.length : 0, sql);
514517
try {

src/test/java/ai/spice/ResetTest.java

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@ of this software and associated documentation files (the "Software"), to deal
2323
package ai.spice;
2424

2525
import java.net.URI;
26-
import java.util.ArrayList;
2726
import java.util.List;
27+
import java.util.concurrent.CopyOnWriteArrayList;
2828
import java.util.concurrent.CountDownLatch;
2929
import java.util.concurrent.ExecutorService;
3030
import java.util.concurrent.Executors;
@@ -42,18 +42,27 @@ of this software and associated documentation files (the "Software"), to deal
4242
*/
4343
public class ResetTest extends TestCase {
4444

45-
private boolean serverAvailable = false;
45+
private static volatile boolean serverAvailable = false;
46+
private static volatile boolean serverAvailabilityChecked = false;
4647

4748
@Override
4849
protected void setUp() throws Exception {
4950
super.setUp();
50-
try (SpiceClient probe = SpiceClient.builder().build()) {
51-
try (FlightStream stream = probe.query("SELECT 1")) {
52-
stream.next();
51+
if (!serverAvailabilityChecked) {
52+
synchronized (ResetTest.class) {
53+
if (!serverAvailabilityChecked) {
54+
try (SpiceClient probe = SpiceClient.builder().build()) {
55+
try (FlightStream stream = probe.query("SELECT 1")) {
56+
stream.next();
57+
}
58+
serverAvailable = true;
59+
} catch (Exception e) {
60+
serverAvailable = false;
61+
} finally {
62+
serverAvailabilityChecked = true;
63+
}
64+
}
5365
}
54-
serverAvailable = true;
55-
} catch (Exception e) {
56-
serverAvailable = false;
5766
}
5867
}
5968

@@ -257,7 +266,7 @@ public void testConcurrentResetAndQuery() throws Exception {
257266
final SpiceClient client = SpiceClient.builder().build();
258267
final int iterations = 5;
259268
final CountDownLatch startLatch = new CountDownLatch(1);
260-
final List<Throwable> unexpectedErrors = new ArrayList<>();
269+
final List<Throwable> unexpectedErrors = new CopyOnWriteArrayList<>();
261270
ExecutorService executor = Executors.newFixedThreadPool(2);
262271

263272
try {

0 commit comments

Comments
 (0)