Skip to content

Commit 5a464d8

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 5a464d8

2 files changed

Lines changed: 25 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: 22 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,31 @@ 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+
// Probe with taxi_trips (not SELECT 1) to ensure
56+
// the dataset is loaded and ready, not just that
57+
// the server is up.
58+
try (FlightStream stream = probe.query(
59+
"SELECT total_amount FROM taxi_trips LIMIT 1")) {
60+
stream.next();
61+
}
62+
serverAvailable = true;
63+
} catch (Exception e) {
64+
serverAvailable = false;
65+
} finally {
66+
serverAvailabilityChecked = true;
67+
}
68+
}
5369
}
54-
serverAvailable = true;
55-
} catch (Exception e) {
56-
serverAvailable = false;
5770
}
5871
}
5972

@@ -257,7 +270,7 @@ public void testConcurrentResetAndQuery() throws Exception {
257270
final SpiceClient client = SpiceClient.builder().build();
258271
final int iterations = 5;
259272
final CountDownLatch startLatch = new CountDownLatch(1);
260-
final List<Throwable> unexpectedErrors = new ArrayList<>();
273+
final List<Throwable> unexpectedErrors = new CopyOnWriteArrayList<>();
261274
ExecutorService executor = Executors.newFixedThreadPool(2);
262275

263276
try {

0 commit comments

Comments
 (0)