From d1c27feb32f95f20ad3d513c68084925fe10de8f Mon Sep 17 00:00:00 2001 From: Luke Kim <80174+lukekim@users.noreply.github.com> Date: Thu, 30 Jul 2026 12:10:43 -0700 Subject: [PATCH 1/2] =?UTF-8?q?docs:=20v0.7.0=20release=20preparation=20?= =?UTF-8?q?=E2=80=94=20release=20notes,=20upgrade=20guide,=20QA=20hardenin?= =?UTF-8?q?g?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Release notes now cover the full v0.6.0..v0.7.0 delta: mTLS client certificates (#46) and health/readiness/status checks (#48) were never released and join the perf overhaul (#50); dependency table corrected against what v0.6.0 actually shipped (ADBC 0.22.0) - New upgrade guide: docs/upgrade_guides/v0.6.0-to-v0.7.0.md (behavior changes, dependency-tree impacts, new capabilities) - README installation snippets bumped to 0.7.0 - Integration-test hardening found during live-runtime QA: - testRefreshWithOptionsSpiceOSS was rerun-unsafe: its refresh_sql LIMIT persists in the accelerated table across runs and refreshes are async. Now self-heals the precondition, polls instead of fixed sleep, and restores the dataset afterward - availability probes use one retry instead of zero (resilient gating, still fast when no runtime is present) QA: 284 tests green twice consecutively against a live spiced quickstart (taxi_trips, 2.9M rows) plus mock-only runs; jar/sources/javadoc build clean. --- README.md | 4 +- docs/release_notes/v0.7.0.md | 73 +++++++++--- docs/upgrade_guides/v0.6.0-to-v0.7.0.md | 110 ++++++++++++++++++ src/test/java/ai/spice/FlightQueryTest.java | 63 ++++++---- .../java/ai/spice/ParameterizedQueryTest.java | 14 +-- src/test/java/ai/spice/ResetTest.java | 2 +- .../java/ai/spice/TpchIntegrationTest.java | 2 +- 7 files changed, 218 insertions(+), 50 deletions(-) create mode 100644 docs/upgrade_guides/v0.6.0-to-v0.7.0.md diff --git a/README.md b/README.md index a501371..fa9c8eb 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ Add the following dependency to your Maven project: ai.spice spiceai - 0.6.0 + 0.7.0 compile ``` @@ -22,7 +22,7 @@ Add the following dependency to your Maven project: Add the following dependency to your Gradle project: ```groovy -implementation 'ai.spice:spiceai:0.6.0' +implementation 'ai.spice:spiceai:0.7.0' ``` ### Manual installation diff --git a/docs/release_notes/v0.7.0.md b/docs/release_notes/v0.7.0.md index 150c974..4f57df5 100644 --- a/docs/release_notes/v0.7.0.md +++ b/docs/release_notes/v0.7.0.md @@ -2,15 +2,46 @@ ## Highlights -This release is a **performance overhaul**. Parameterized queries move off ADBC onto native Arrow Flight SQL prepared statements running on the same tuned connection as regular queries, prepared statements are cached and reused, retries use real exponential backoff, and the Netty/gRPC dependency stack is aligned onto supported versions. +v0.7.0 is a **performance and operability release**. Parameterized queries move off ADBC onto native Arrow Flight SQL prepared statements running on the same tuned connection as regular queries, prepared statements are cached and reused, retries use real exponential backoff, and the Netty/gRPC dependency stack is aligned onto supported versions. The release also delivers **mTLS client certificates** and **health, readiness, and runtime status checks**, both landing in a release for the first time. + +Most applications can upgrade without code changes โ€” see the [upgrade guide](../upgrade_guides/v0.6.0-to-v0.7.0.md). ## What's New +### ๐Ÿ” mTLS Client Certificates + +Connect to Spice deployments that require mutual TLS, and/or trust a custom certificate authority. PEM-encoded files are configured on the builder and apply to Flight (gRPC) queries, parameterized queries, and HTTP operations alike: + +```java +SpiceClient client = SpiceClient.builder() + .withFlightAddress(new URI("grpc+tls://spice.example.com:50051")) + .withTlsClientCertFile("/etc/certs/client.pem") // client identity + .withTlsClientKeyFile("/etc/certs/client-key.pem") + .withTlsRootCertFile("/etc/certs/ca.pem") // custom CA (optional) + .build(); +``` + +### ๐Ÿฅ Health, Readiness, and Runtime Status + +Probe the runtime before sending queries, and introspect per-component state: + +```java +if (!client.isHealthy()) { /* runtime is down โ€” liveness probe (/health) */ } +if (!client.isReady()) { /* datasets still loading โ€” readiness probe (/v1/ready) */ } + +// Per-connection detail (/v1/status): http, flight, metrics, opentelemetry +for (ConnectionDetails details : client.runtimeStatus()) { + System.out.println(details.getName() + ": " + details.getStatus()); +} +``` + +`isHealthy()`/`isReady()` return booleans (never throw on an unreachable runtime) so they can be polled directly in startup loops. See the [health and status example](/src/main/java/ai/spice/example/ExampleHealthAndStatus.java). + ### โšก Native Flight SQL Prepared Statements (ADBC removed) `queryWithParams` is now implemented directly on Arrow Flight SQL prepared statements instead of the ADBC driver. This is a pure win with no API change โ€” the method still returns an `ArrowReader`: -- **One connection instead of two.** Parameterized queries previously ran on a separate ADBC-managed gRPC channel that had none of the SDK's transport tuning. They now share the primary channels, inheriting DNS re-resolution (`dns:///`), HTTP/2 keep-alive, and โ€” new in this release for parameterized queries โ€” **mTLS/custom CA support**. +- **One connection instead of two.** Parameterized queries previously ran on a separate ADBC-managed gRPC channel that had none of the SDK's transport tuning. They now share the primary channels, inheriting DNS re-resolution (`dns:///`), HTTP/2 keep-alive, and mTLS. - **Prepared statement caching.** Statements are cached per SQL string and reused across executions, removing the `CreatePreparedStatement` and `ClosePreparedStatement` round trips from every repeated query (5 โ†’ 3 round trips per call). Configure with `withPreparedStatementCacheSize(n)` (default 64, `0` disables). - **Transparent recovery.** If the server no longer recognizes a cached statement handle (e.g. after a server restart), the SDK re-prepares and retries automatically. - **Multi-endpoint results.** The `ArrowReader` consumes every endpoint of a partitioned result. @@ -34,44 +65,52 @@ SpiceClient client = SpiceClient.builder() Retries previously waited 1โ€“2ms between attempts โ€” useless against real outages. They now use exponential backoff with jitter (~250ms, 500ms, 1s, โ€ฆ capped at 10s), so the default 3 retries cover ~2s outages such as load-balancer failovers. -### ๐Ÿ” Automatic Re-Authentication +### ๐Ÿ”„ Automatic Re-Authentication If the server reports `UNAUTHENTICATED` (e.g. an expired handshake bearer token on a long-lived client), the SDK re-handshakes once and retries the query automatically. Previously this required a manual `reset()`. +### ๐Ÿ›ก๏ธ Safer reset() Under Concurrency + +`reset()` (and the automatic re-authentication above) now *retires* old connections gracefully instead of closing them inline: in-flight queries and open result streams complete on the old transport while new queries move to fresh connections. Previously a concurrent query could fail with an allocator error, and a race with `queryWithParams` could throw `NullPointerException`. + +### ๐Ÿ HikariCP / JDBC Interop + +`SpiceClient` is thread-safe and needs no external pool (use `withChannelCount`), but applications with JDBC infrastructure can pool connections to Spice with HikariCP via the Arrow Flight SQL JDBC driver. This combination โ€” including authentication and prepared statements โ€” is verified by `HikariCpCompatTest`, and the README documents the recommended configuration. + ### ๐Ÿงน Smaller Fixes -- Fixed a race where `reset()` concurrent with `queryWithParams` could throw `NullPointerException`. - Parameter bind roots are sized for their single row instead of Arrow's ~3970-slot default (~48KB saved per string parameter per query). - `refreshDataset` now sets a 60s request timeout (previously it could hang forever) and the HTTP client is created lazily. - The user agent string is computed once instead of per request; fixed the OS version reported on Windows. -- `query()` logs a warning if the server returns a multi-endpoint result (only the first endpoint is consumed by the `FlightStream` API; use `queryWithParams` to consume all). +- `query()` logs a warning if the server returns a multi-endpoint result (only the first endpoint is consumed by the `FlightStream` API; use `queryWithParams` to consume all), and raises a clear error if the server returns no endpoints. ## Dependency Changes | Dependency | v0.6.0 | v0.7.0 | | ----------------------------------- | ------------- | ------------ | -| Apache Arrow ADBC Driver Flight SQL | 0.23.0 | **removed** | -| Apache Arrow ADBC Core | 0.23.0 | **removed** | +| Apache Arrow Flight SQL | 19.0.0 | 19.0.0 | +| Apache Arrow ADBC Driver Flight SQL | 0.22.0 | **removed** | +| Apache Arrow ADBC Core | 0.22.0 | **removed** | | netty-all | 4.2.12.Final | **removed** | -| Netty (via netty-bom) | 4.2.12.Final (forced) | 4.1.130.Final | -| gRPC (via grpc-bom) | 1.79.0 | 1.79.0 (pinned) | +| Netty (via netty-bom) | 4.2.12.Final (forced by netty-all) | 4.1.130.Final | +| gRPC (via grpc-bom) | transitive | 1.79.0 (pinned) | | netty-transport-native-epoll | โ€” | 4.1.130.Final (Linux runtime) | -| BouncyCastle (bcprov/bcpkix) | transitive | 1.80 (explicit) | +| BouncyCastle (bcprov/bcpkix) | transitive via ADBC | 1.80 (explicit) | -The `netty-all 4.2.12` pin forced every Netty module to the 4.2 line, which `grpc-netty 1.79` (built against Netty 4.1.130) does not support, and dragged unused codecs (HTTP/3, MQTT, SMTP, Redis, โ€ฆ) onto consumers' classpaths. The stack is now aligned on Netty 4.1.130.Final via `netty-bom`, with native epoll transports added for Linux. BouncyCastle (used for mTLS PEM parsing) is now an explicit dependency instead of riding on ADBC's transitives. - -### ๐Ÿ HikariCP / JDBC Interop +The `netty-all 4.2.12` dependency forced every Netty module to the 4.2 line, which `grpc-netty 1.79` (built against Netty 4.1.130) does not support, and dragged unused codecs (HTTP/3, MQTT, SMTP, Redis, โ€ฆ) onto consumers' classpaths. The stack is now aligned on Netty 4.1.130.Final via `netty-bom`, with native epoll transports added for Linux. BouncyCastle (used for mTLS PEM parsing) is now an explicit dependency instead of riding on ADBC's transitives. -`SpiceClient` is thread-safe and needs no external pool (use `withChannelCount`), but applications with JDBC infrastructure can pool connections to Spice with HikariCP via the Arrow Flight SQL JDBC driver. This combination โ€” including authentication and prepared statements โ€” is now verified by `HikariCpCompatTest`, and the README documents the recommended configuration. +Test-scope additions (not shipped to consumers): HikariCP 5.1.0 and `flight-sql-jdbc-core` 19.0.0 for the JDBC interop tests. ## Testing -- New in-process mock Flight SQL server (`TestFlightSqlServer`) with per-RPC counters and failure injection โ€” the full query, prepared-statement, retry, timeout, re-auth, and multi-endpoint behavior is now covered without an external runtime. -- New test suites: `LocalFlightServerTest`, `StatementCacheTest`, `ResilienceTest`, `FlightInfoReaderTest`, `ParamRootTest`, `RefreshHttpTest`, `HikariCpCompatTest`. +- New in-process mock Flight SQL server (`TestFlightSqlServer`) with per-RPC counters and failure injection โ€” query, prepared-statement, retry, timeout, re-auth, and multi-endpoint behavior is now covered without an external runtime. +- New test suites: `LocalFlightServerTest`, `StatementCacheTest`, `ResilienceTest`, `FlightInfoReaderTest`, `ParamRootTest`, `RefreshHttpTest`, `HikariCpCompatTest`, `RuntimeStatusTest`. - New `PerfBenchmarkTest` printing latency percentiles and asserting the round-trip contract (cached path: zero prepares per query; uncached: one prepare + one close per query). +- 284 tests in total. ## Compatibility -- Public API unchanged: `query`, `queryWithParams`, `refreshDataset`, `reset`, `close`, and all existing builder methods behave as before. +- Public API unchanged: `query`, `queryWithParams`, `refreshDataset`, `reset`, `close`, and all existing builder methods behave as before. New APIs are additive. - `queryWithParams` error causes are now `FlightRuntimeException` instead of `AdbcException` (both surfaced inside `ExecutionException`). - To restore the previous prepare-per-query behavior: `withPreparedStatementCacheSize(0)`. +- Full details: [upgrade guide](../upgrade_guides/v0.6.0-to-v0.7.0.md). diff --git a/docs/upgrade_guides/v0.6.0-to-v0.7.0.md b/docs/upgrade_guides/v0.6.0-to-v0.7.0.md new file mode 100644 index 0000000..e796464 --- /dev/null +++ b/docs/upgrade_guides/v0.6.0-to-v0.7.0.md @@ -0,0 +1,110 @@ +# Upgrading from v0.6.0 to v0.7.0 + +**TL;DR: for most applications this is a drop-in upgrade.** Bump the version, rebuild, done: + +```xml + + ai.spice + spiceai + 0.7.0 + +``` + +The public API is unchanged and all new capabilities are additive. Read on for the behavior changes worth knowing about, the dependency-tree changes that can affect applications with their own Arrow/Netty usage, and the new features you may want to adopt. + +## Behavior changes + +### Prepared statement caching is on by default + +`queryWithParams` now caches server-side prepared statements per SQL string (up to 64 idle statements) and reuses them across calls, saving two network round trips per repeated query. Statements are closed on `reset()` and `close()`, and stale handles (e.g. after a server restart) transparently re-prepare. + +- Impact: the Spice runtime will show long-lived prepared statements for clients that repeat queries. This is intended. +- Opt out (restores the v0.6.0 prepare-per-query behavior): + +```java +SpiceClient.builder().withPreparedStatementCacheSize(0).build(); +``` + +### Retries now take real time + +v0.6.0 retried failed queries after 1โ€“2ms waits, which effectively meant no recovery window at all. v0.7.0 uses exponential backoff with jitter (~250ms, 500ms, 1s, โ€ฆ capped at 10s). + +- Impact: with the default `withMaxRetries(3)`, a query against an unavailable runtime now surfaces its error after roughly 2 seconds instead of near-instantly. This is what makes retries actually survive load-balancer failovers and restarts. +- If you need fail-fast behavior (e.g. latency-sensitive fallback paths), set `withMaxRetries(0)` and handle retries yourself, or set `withQueryTimeout(...)`. + +### Exception causes for parameterized queries changed type + +`queryWithParams` still throws `java.util.concurrent.ExecutionException`, but the *cause* is now an `org.apache.arrow.flight.FlightRuntimeException` (previously `org.apache.arrow.adbc.core.AdbcException`, which no longer exists on the classpath). + +```java +// Before (v0.6.0) +catch (ExecutionException e) { + if (e.getCause() instanceof AdbcException) { ... } +} + +// After (v0.7.0) +catch (ExecutionException e) { + if (e.getCause() instanceof FlightRuntimeException) { + FlightStatusCode code = ((FlightRuntimeException) e.getCause()).status().code(); + ... + } +} +``` + +If you never inspected the cause type, nothing changes. + +### Expired auth tokens recover automatically + +Long-lived authenticated clients whose handshake bearer token expires no longer fail until a manual `reset()`: the SDK re-handshakes once on `UNAUTHENTICATED` and retries. Remove any workaround code that called `reset()` for this case (it stays useful for other stuck-transport situations). + +### reset() is gentler to concurrent queries + +`reset()` now retires old connections gracefully โ€” queries already in flight complete on the old transport, new queries use fresh connections. In v0.6.0, concurrent queries could fail with allocator errors or (in a narrow race) `NullPointerException`. + +### refreshDataset has a request timeout + +`refreshDataset` now fails after 60 seconds instead of potentially hanging forever on an unresponsive runtime. + +## Dependency changes that can affect your build + +The SDK's dependency tree changed significantly. If your application only uses the SDK's public API, no action is needed. Check the cases below if you use Arrow, Netty, ADBC, or BouncyCastle directly: + +| Change | Who is affected | Action | +| ------ | --------------- | ------ | +| **ADBC removed** (`adbc-driver-flight-sql`, `adbc-core`) | Applications importing `org.apache.arrow.adbc.*` classes that arrived transitively through this SDK | Declare the ADBC artifacts in your own build | +| **netty-all removed** | Applications relying on Netty modules (codecs, transports) that arrived transitively via `netty-all` | Declare the specific Netty modules you use | +| **Netty now 4.1.130.Final** (was a forced 4.2.12) | Applications pinning their own Netty version | 4.1.130.Final is the line `grpc-netty 1.79` supports; avoid forcing 4.2.x onto the SDK's classpath | +| **gRPC pinned to 1.79.0** via `grpc-bom` | Applications pinning their own gRPC version | Align on 1.79.x, or verify your version against Netty 4.1.130 | +| **BouncyCastle explicit at 1.80** (`bcprov-jdk18on`, `bcpkix-jdk18on`) | Applications with their own BouncyCastle pin | Maven nearest-wins applies as usual; 1.80 is only required for the SDK's mTLS PEM parsing | +| **netty-transport-native-epoll added** (Linux classifiers, runtime scope) | Nobody adversely โ€” enables epoll on Linux for lower-latency I/O | None | + +## New capabilities worth adopting + +```java +SpiceClient client = SpiceClient.builder() + // mTLS / custom CA (PEM files) โ€” applies to queries, parameterized queries, and HTTP + .withTlsClientCertFile("/etc/certs/client.pem") + .withTlsClientKeyFile("/etc/certs/client-key.pem") + .withTlsRootCertFile("/etc/certs/ca.pem") + // Connection pool for highly concurrent workloads (default 1) + .withChannelCount(4) + // Deadline for query planning / prepare / bind RPCs (not result streaming) + .withQueryTimeout(Duration.ofSeconds(30)) + // Prepared statement reuse (default 64; 0 disables) + .withPreparedStatementCacheSize(128) + .build(); + +// Health and readiness probes (never throw; poll them in startup loops) +client.isHealthy(); // GET /health โ€” liveness +client.isReady(); // GET /v1/ready โ€” datasets loaded +client.runtimeStatus(); // GET /v1/status โ€” per-connection detail + +// Kubernetes-style wait-for-ready +while (!client.isReady()) { Thread.sleep(500); } +``` + +For JDBC ecosystems (ORMs, HikariCP): the SDK itself needs no connection pool, but pooling JDBC connections to Spice via the Arrow Flight SQL JDBC driver is supported and tested โ€” see "Connection Pooling and HikariCP" in the README. + +## Supported Java versions + +Unchanged: Java 11+ (see the README support matrix for the tested JDK distributions). diff --git a/src/test/java/ai/spice/FlightQueryTest.java b/src/test/java/ai/spice/FlightQueryTest.java index 9d4f3f2..fbc2c9a 100644 --- a/src/test/java/ai/spice/FlightQueryTest.java +++ b/src/test/java/ai/spice/FlightQueryTest.java @@ -78,9 +78,9 @@ public void testQuerySpiceCloudPlatform() throws ExecutionException, Interrupted public void testQuerySpiceOSS() throws ExecutionException, InterruptedException { try { - // maxRetries=0: keep the no-local-server skip path fast under real backoff + // One retry: resilient availability detection without paying full backoff when absent SpiceClient spiceClient = SpiceClient.builder() - .withMaxRetries(0) + .withMaxRetries(1) .build(); String sql = "SELECT tpep_pickup_datetime, total_amount, passenger_count from taxi_trips limit 10;"; @@ -140,35 +140,28 @@ public void testRefreshWithOptionsSpiceOSS() throws ExecutionException, Interrup SpiceClient spiceClient = SpiceClient.builder() .build(); - FlightStream preRefreshRes = spiceClient.query(sql); - - int preRefreshRows = 0; - - while (preRefreshRes.next()) { - VectorSchemaRoot root = preRefreshRes.getRoot(); - preRefreshRows += root.getRowCount(); + // A previous run of this test leaves the accelerated table at 10 rows + // (refreshes are asynchronous and the refresh_sql persists in the + // acceleration). Restore the full dataset first so the pre-condition + // holds on reused runtimes, not only on freshly started ones. + if (countRows(spiceClient, sql) < 20) { + spiceClient.refreshDataset("taxi_trips"); + waitForRowCount(spiceClient, sql, 20); } - assertEquals("Expected row count does not match", 20, preRefreshRows); + assertEquals("Expected row count does not match", 20, countRows(spiceClient, sql)); RefreshOptions opts = new RefreshOptions().withRefreshSql("SELECT * FROM taxi_trips limit 10") .withRefreshJitterMax("1s"); spiceClient.refreshDataset("taxi_trips", opts); - // wait a couple seconds to let refresh run - Thread.sleep(10000); - - FlightStream postRefreshRes = spiceClient.query(sql); - - int postRefreshRows = 0; + // Refreshes are asynchronous: poll until the shrunk dataset is visible + // instead of relying on a fixed sleep. + assertEquals("Expected row count does not match", 10, waitForRowCount(spiceClient, sql, 10)); - while (postRefreshRes.next()) { - VectorSchemaRoot root = postRefreshRes.getRoot(); - postRefreshRows += root.getRowCount(); - } - - assertEquals("Expected row count does not match", 10, postRefreshRows); + // Leave the dataset restored for other tests and later runs (async). + spiceClient.refreshDataset("taxi_trips"); } catch (Exception e) { // Skip if table not found, connection unavailable, or acceleration not ready String msg = e.getMessage() != null ? e.getMessage().toLowerCase() : ""; @@ -178,4 +171,30 @@ public void testRefreshWithOptionsSpiceOSS() throws ExecutionException, Interrup fail("Should not throw exception: " + e.getMessage()); } } + + private static long countRows(SpiceClient client, String sql) throws Exception { + try (FlightStream stream = client.query(sql)) { + long rows = 0; + while (stream.next()) { + rows += stream.getRoot().getRowCount(); + } + return rows; + } + } + + /** + * Polls (up to 30s) until the query returns the expected row count, + * returning the last observed count either way. + */ + private static long waitForRowCount(SpiceClient client, String sql, long expected) throws Exception { + long rows = -1; + for (int i = 0; i < 30; i++) { + rows = countRows(client, sql); + if (rows == expected) { + return rows; + } + Thread.sleep(1000); + } + return rows; + } } diff --git a/src/test/java/ai/spice/ParameterizedQueryTest.java b/src/test/java/ai/spice/ParameterizedQueryTest.java index b06d1c1..be65400 100644 --- a/src/test/java/ai/spice/ParameterizedQueryTest.java +++ b/src/test/java/ai/spice/ParameterizedQueryTest.java @@ -82,7 +82,7 @@ public void testParameterizedQuerySpiceCloud() throws Exception { * Test parameterized query with local Spice OSS runtime. */ public void testParameterizedQuerySpiceOSS() throws Exception { - try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(0).build()) { + try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(1).build()) { // Test with float parameter on tpch.orders String sql = "SELECT o_orderkey, o_totalprice FROM tpch.orders WHERE o_totalprice > $1 ORDER BY o_totalprice LIMIT 5"; @@ -113,7 +113,7 @@ public void testParameterizedQuerySpiceOSS() throws Exception { * Test parameterized query with multiple parameters. */ public void testMultipleParameters() throws Exception { - try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(0).build()) { + try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(1).build()) { String sql = "SELECT o_orderkey, o_totalprice FROM tpch.orders WHERE o_totalprice > $1 AND o_custkey > $2 LIMIT 5"; try (ArrowReader reader = spiceClient.queryWithParams(sql, 5000.0, 100)) { @@ -140,7 +140,7 @@ public void testMultipleParameters() throws Exception { * Test parameterized query with string parameter. */ public void testStringParameter() throws Exception { - try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(0).build()) { + try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(1).build()) { // Use c_mktsegment which is a string column in tpch.customer String sql = "SELECT c_custkey, c_mktsegment FROM tpch.customer WHERE c_mktsegment = $1 LIMIT 5"; @@ -168,7 +168,7 @@ public void testStringParameter() throws Exception { * Test parameterized query with explicit Param types. */ public void testExplicitParamTypes() throws Exception { - try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(0).build()) { + try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(1).build()) { // Use explicit int64 type on tpch.customer String sql = "SELECT c_custkey, c_name, c_nationkey FROM tpch.customer WHERE c_nationkey = $1 LIMIT 5"; @@ -195,7 +195,7 @@ public void testExplicitParamTypes() throws Exception { * Test parameterized query with mixed parameter types. */ public void testMixedParameterTypes() throws Exception { - try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(0).build()) { + try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(1).build()) { String sql = "SELECT o_orderkey, o_totalprice FROM tpch.orders WHERE o_totalprice > $1 AND o_orderstatus = $2 LIMIT 5"; try (ArrowReader reader = spiceClient.queryWithParams(sql, @@ -290,7 +290,7 @@ public void testParamFactoryMethods() { * Test that null SQL throws IllegalArgumentException. */ public void testNullSqlThrows() throws Exception { - try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(0).build()) { + try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(1).build()) { try { spiceClient.queryWithParams(null, 1); fail("Expected IllegalArgumentException"); @@ -310,7 +310,7 @@ public void testNullSqlThrows() throws Exception { * Test that empty SQL throws IllegalArgumentException. */ public void testEmptySqlThrows() throws Exception { - try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(0).build()) { + try (SpiceClient spiceClient = SpiceClient.builder().withMaxRetries(1).build()) { try { spiceClient.queryWithParams("", 1); fail("Expected IllegalArgumentException"); diff --git a/src/test/java/ai/spice/ResetTest.java b/src/test/java/ai/spice/ResetTest.java index 75fe4f0..6127f77 100644 --- a/src/test/java/ai/spice/ResetTest.java +++ b/src/test/java/ai/spice/ResetTest.java @@ -53,7 +53,7 @@ protected void setUp() throws Exception { if (!serverAvailabilityChecked) { synchronized (ResetTest.class) { if (!serverAvailabilityChecked) { - try (SpiceClient probe = SpiceClient.builder().withMaxRetries(0).build()) { + try (SpiceClient probe = SpiceClient.builder().withMaxRetries(1).build()) { // Probe with taxi_trips (not SELECT 1) to ensure // the dataset is loaded and ready, not just that // the server is up. diff --git a/src/test/java/ai/spice/TpchIntegrationTest.java b/src/test/java/ai/spice/TpchIntegrationTest.java index 9c2fb6d..88c59c6 100644 --- a/src/test/java/ai/spice/TpchIntegrationTest.java +++ b/src/test/java/ai/spice/TpchIntegrationTest.java @@ -58,7 +58,7 @@ protected void setUp() throws Exception { if (tpchAvailableCached == null) { synchronized (TpchIntegrationTest.class) { if (tpchAvailableCached == null) { - try (SpiceClient probe = SpiceClient.builder().withMaxRetries(0).build(); + try (SpiceClient probe = SpiceClient.builder().withMaxRetries(1).build(); FlightStream stream = probe.query("SELECT c_custkey FROM tpch.customer LIMIT 1")) { stream.next(); tpchAvailableCached = Boolean.TRUE; From 6152f9e74768abb1b45ae4282a33d277fc3699a0 Mon Sep 17 00:00:00 2001 From: Luke Kim <80174+lukekim@users.noreply.github.com> Date: Thu, 30 Jul 2026 14:21:28 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20test=20resource=20management=20and=20comment=20accu?= =?UTF-8?q?racy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - FlightQueryTest: all four tests now close SpiceClient (and FlightStream) via try-with-resources - testRefreshWithOptionsSpiceOSS: shrink/verify runs in try/finally with a guaranteed restore that waits for the async refresh to land; restore failures are logged, never masking the primary test failure - TpchIntegrationTest: probe comment updated to match withMaxRetries(1) Validated against a live spiced quickstart: FlightQueryTest green twice consecutively with the dataset restored to full row count after each run; full suite 284/284 + SpotBugs clean. --- src/test/java/ai/spice/FlightQueryTest.java | 91 ++++++++++--------- .../java/ai/spice/TpchIntegrationTest.java | 4 +- 2 files changed, 51 insertions(+), 44 deletions(-) diff --git a/src/test/java/ai/spice/FlightQueryTest.java b/src/test/java/ai/spice/FlightQueryTest.java index fbc2c9a..56197a5 100644 --- a/src/test/java/ai/spice/FlightQueryTest.java +++ b/src/test/java/ai/spice/FlightQueryTest.java @@ -42,25 +42,24 @@ public void testQuerySpiceCloudPlatform() throws ExecutionException, Interrupted return; } - try { - SpiceClient spiceClient = SpiceClient.builder() - .withApiKey(apiKey) // https://spice.ai/spiceai/quickstart - .withHttpAddress(new URI("https://data.spiceai.io")) - .withFlightAddress(new URI("https://flight.spiceai.io:443")) - .build(); + try (SpiceClient spiceClient = SpiceClient.builder() + .withApiKey(apiKey) // https://spice.ai/spiceai/quickstart + .withHttpAddress(new URI("https://data.spiceai.io")) + .withFlightAddress(new URI("https://flight.spiceai.io:443")) + .build()) { String sql = "SELECT tpep_pickup_datetime, total_amount, passenger_count from taxi_trips limit 10;"; - FlightStream res = spiceClient.query(sql); - int totalRows = 0; int columnCount = 0; - while (res.next()) { - VectorSchemaRoot root = res.getRoot(); - if (totalRows == 0) { - columnCount = root.getFieldVectors().size(); + try (FlightStream res = spiceClient.query(sql)) { + while (res.next()) { + VectorSchemaRoot root = res.getRoot(); + if (totalRows == 0) { + columnCount = root.getFieldVectors().size(); + } + totalRows += root.getRowCount(); } - totalRows += root.getRowCount(); } assertEquals("Expected column count does not match", 3, columnCount); @@ -77,24 +76,23 @@ public void testQuerySpiceCloudPlatform() throws ExecutionException, Interrupted } public void testQuerySpiceOSS() throws ExecutionException, InterruptedException { - try { - // One retry: resilient availability detection without paying full backoff when absent - SpiceClient spiceClient = SpiceClient.builder() - .withMaxRetries(1) - .build(); + // One retry: resilient availability detection without paying full backoff when absent + try (SpiceClient spiceClient = SpiceClient.builder() + .withMaxRetries(1) + .build()) { String sql = "SELECT tpep_pickup_datetime, total_amount, passenger_count from taxi_trips limit 10;"; - FlightStream res = spiceClient.query(sql); - int totalRows = 0; int columnCount = 0; - while (res.next()) { - VectorSchemaRoot root = res.getRoot(); - if (totalRows == 0) { - columnCount = root.getFieldVectors().size(); + try (FlightStream res = spiceClient.query(sql)) { + while (res.next()) { + VectorSchemaRoot root = res.getRoot(); + if (totalRows == 0) { + columnCount = root.getFieldVectors().size(); + } + totalRows += root.getRowCount(); } - totalRows += root.getRowCount(); } assertEquals("Expected column count does not match", 3, columnCount); @@ -111,9 +109,8 @@ public void testQuerySpiceOSS() throws ExecutionException, InterruptedException } public void testRefreshSpiceOSS() throws ExecutionException, InterruptedException { - try { - SpiceClient spiceClient = SpiceClient.builder() - .build(); + try (SpiceClient spiceClient = SpiceClient.builder() + .build()) { spiceClient.refreshDataset("taxi_trips"); @@ -135,10 +132,9 @@ public void testRefreshSpiceOSS() throws ExecutionException, InterruptedExceptio } public void testRefreshWithOptionsSpiceOSS() throws ExecutionException, InterruptedException { - try { + try (SpiceClient spiceClient = SpiceClient.builder() + .build()) { String sql = "SELECT tpep_pickup_datetime, total_amount, passenger_count from taxi_trips limit 20;"; - SpiceClient spiceClient = SpiceClient.builder() - .build(); // A previous run of this test leaves the accelerated table at 10 rows // (refreshes are asynchronous and the refresh_sql persists in the @@ -151,17 +147,28 @@ public void testRefreshWithOptionsSpiceOSS() throws ExecutionException, Interrup assertEquals("Expected row count does not match", 20, countRows(spiceClient, sql)); - RefreshOptions opts = new RefreshOptions().withRefreshSql("SELECT * FROM taxi_trips limit 10") - .withRefreshJitterMax("1s"); - - spiceClient.refreshDataset("taxi_trips", opts); - - // Refreshes are asynchronous: poll until the shrunk dataset is visible - // instead of relying on a fixed sleep. - assertEquals("Expected row count does not match", 10, waitForRowCount(spiceClient, sql, 10)); - - // Leave the dataset restored for other tests and later runs (async). - spiceClient.refreshDataset("taxi_trips"); + try { + RefreshOptions opts = new RefreshOptions().withRefreshSql("SELECT * FROM taxi_trips limit 10") + .withRefreshJitterMax("1s"); + + spiceClient.refreshDataset("taxi_trips", opts); + + // Refreshes are asynchronous: poll until the shrunk dataset is visible + // instead of relying on a fixed sleep. + assertEquals("Expected row count does not match", 10, waitForRowCount(spiceClient, sql, 10)); + } finally { + // Always restore the full dataset and wait for it to land, so + // subsequent tests and reruns see the standard state even when + // the assertions above fail. Best-effort: a restore failure must + // not mask the primary test failure. + try { + spiceClient.refreshDataset("taxi_trips"); + waitForRowCount(spiceClient, sql, 20); + } catch (Exception restoreFailure) { + System.err.println("Warning: failed to restore taxi_trips after refresh test: " + + restoreFailure.getMessage()); + } + } } catch (Exception e) { // Skip if table not found, connection unavailable, or acceleration not ready String msg = e.getMessage() != null ? e.getMessage().toLowerCase() : ""; diff --git a/src/test/java/ai/spice/TpchIntegrationTest.java b/src/test/java/ai/spice/TpchIntegrationTest.java index 88c59c6..52f3b85 100644 --- a/src/test/java/ai/spice/TpchIntegrationTest.java +++ b/src/test/java/ai/spice/TpchIntegrationTest.java @@ -48,8 +48,8 @@ public class TpchIntegrationTest extends TestCase { private SpiceClient client; private boolean tpchAvailable = true; - // Availability is probed once for the whole class (with retries disabled) - // so a missing local runtime doesn't cost retry backoff per test method. + // Availability is probed once for the whole class (with a single retry) + // so a missing local runtime doesn't cost full retry backoff per test method. private static volatile Boolean tpchAvailableCached; @Override