Repository navigation
fix(bigquery-jdbc): stop producer and release resources on ResultSet.close - #14712
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves resource cleanup, concurrency handling, and exception safety across BigQueryArrowResultSet, BigQueryJsonResultSet, and BigQueryStatement. Key changes include adding idempotency checks and resource cleanup in result set close() methods, replacing uninterruptible queue operations with standard interruptible operations, and introducing a helper method isWorkerShuttingDown() to check thread and executor states. Feedback suggests wrapping the custom resource cleanup in BigQueryArrowResultSet.close() in a try-finally block to guarantee super.close() is always executed, and leveraging the new isWorkerShuttingDown() helper in BigQueryStatement to simplify checks and avoid potential NullPointerExceptions.
|
|
||
| private final BigQueryJdbcCustomLogger LOG = new BigQueryJdbcCustomLogger(this.toString()); | ||
| public static final int DEFAULT_BUFFER_SIZE = BigQuerySettings.DEFAULT_NUM_BUFFERED_ROWS * 2; | ||
| static final int DEFAULT_ARROW_BUFFER_SIZE = 32; |
There was a problem hiding this comment.
I guess making default 32 makes sense but I wonder if this info should be added/explained somewhere or also if this should be a configurable conn prop?
There was a problem hiding this comment.
What is the rationale behind 32 as the hardcoded number?
There was a problem hiding this comment.
That's an arbitrary constant. If consumer apps is reading data faster than we're fetching, buffer will be ~empty most of the time, if it is slower, we need some limit (similar to how we store 2 pages of results for Json).
We can make it configurable, but I'm not sure there is a lot of value in tweaking it.. Buffer is likely to be either empty or full in either case.
…QueryBaseResultSet" This reverts commit 51cba45.
🤖 I have created a release *beep* *boop* --- <details><summary>1.94.0</summary> ## [1.94.0](v1.93.0...v1.94.0) (2026-10-22) ### Features * **auth:** request certificate-bound tokens for agent identities by ([01eaba1](01eaba1)) * **bigquery-jdbc:** Add clearcut telemetry client ([#14607](#14607)) ([c702438](c702438)) * **bigquery-jdbc:** expose QueryStatistics for jobless queries ([#14596](#14596)) ([15f472c](15f472c)) * **bigquery:** expose QueryStatistics from TableResult ([#14588](#14588)) ([40b2b1a](40b2b1a)) * **bigtable:** add per-method session diversion configuration ([#14681](#14681)) ([042dd3e](042dd3e)) * **bigtable:** support SessionCheckAndMutateRow on the session client ([#14565](#14565)) ([86de9c1](86de9c1)) * **bigtable:** support SessionReadModifyWriteRow on the session client ([#14585](#14585)) ([8e653b2](8e653b2)) * **firestore:** DML ([#14703](#14703)) ([11bc071](11bc071)) * **gax:** implement operation-level client request spans (T3) ([#14549](#14549)) ([275063b](275063b)) * **gax:** link attempt spans to parent context and clean up on operation completion ([#14459](#14459)) ([0c481bd](0c481bd)) * **gax:** standardize mapping terminal resumable upload exceptions to ApiException ([#14624](#14624)) ([d4961f0](d4961f0)) * **google/devicesandservices/health/v4beta:** add health v4beta ([#14600](#14600)) ([95f863c](95f863c)) * **google/shopping/merchant/reviews/v1:** add shopping-merchant-reviews ([#14724](#14724)) ([13310c7](13310c7)) * include writerId as part of trace id ([#13488](#13488)) ([ffa8063](ffa8063)) * log actionable errors on operation failure in LoggingTracer ([#14460](#14460)) ([27559f4](27559f4)) * populate client_stats in AppendRowsRequest ([#14614](#14614)) ([a7a0098](a7a0098)) * **spanner:** add option to create GcpFallbackChannel per channel instead of per pool ([#14501](#14501)) ([74f7591](74f7591)) ### Bug Fixes * **api-common:** validate path template variables against dot segments ([#14641](#14641)) ([4ad59e2](4ad59e2)) * **auth:** fix serialization gaps in DownscopedCredentials and UrlIdentityPoolSubjectTokenSupplier ([#14341](#14341)) ([a3e3324](a3e3324)) * **bigquery-jdbc:** ensure JDBC uses PQC-compliant algorithm by default ([#14560](#14560)) ([b0ffe3d](b0ffe3d)) * **bigquery-jdbc:** fix Exception handling ([#14566](#14566)) ([3e7a206](3e7a206)) * **bigquery-jdbc:** fix missing logger instances ([#14733](#14733)) ([0e822dc](0e822dc)) * **bigquery-jdbc:** fix pqc working in proxy env ([#14599](#14599)) ([ceb45b4](ceb45b4)) * **bigquery-jdbc:** format nested ARRAY fields as valid JSON in Struct.toString() ([#14720](#14720)) ([bc0126a](bc0126a)) * **bigquery-jdbc:** format TIMESTAMP as UTC in getString and add multi-zone TIMESTAMP(12) tests ([#14666](#14666)) ([e666612](e666612)) * **bigquery-jdbc:** honor `Calendar` timezone and prevent `ClassCastException` in ResultSet temporal getters ([#14657](#14657)) ([7dd2b37](7dd2b37)) * **bigquery-jdbc:** limit Storage Read API Arrow batch queue to 32 batches ([#14709](#14709)) ([d339c41](d339c41)) * **bigquery-jdbc:** normalize `setTime` from Calendar zone to JVM zone ([#14660](#14660)) ([54d2bc6](54d2bc6)) * **bigquery-jdbc:** poll the JSON reference queue and release processed finalizers ([#14719](#14719)) ([1b7d00d](1b7d00d)) * **bigquery-jdbc:** preserve `Timestamp` instant in `PreparedStatement` and `CallableStatement` Calendar overloads ([#14640](#14640)) ([3b02a44](3b02a44)) * **bigquery-jdbc:** preserve the instant on the TIMESTAMP write path and bind LocalDateTime as STRING ([#14647](#14647)) ([2106e26](2106e26)) * **bigquery-jdbc:** stop per-ResultSet logger registration leak ([#14718](#14718)) ([e83c768](e83c768)) * **bigquery-jdbc:** stop producer and release resources on ResultSet.close ([#14712](#14712)) ([6b99452](6b99452)) * **bigquery:** fix invalid mappings in domain models ([#14646](#14646)) ([17048ed](17048ed)) * **bigquery:** remove duplicate google-cloud-storage dependency ([#14586](#14586)) ([ab6173e](ab6173e)) * **datastore:** allow any gRPC TransportChannelProvider in DatastoreOptions ([#14478](#14478)) ([d9a8db5](d9a8db5)) * **datastore:** update emulator zip checksum for 2.3.1 ([#14592](#14592)) ([fb73ed5](fb73ed5)) * **deps:** update dependency com.google.api-client:google-api-client-bom to v2.9.2 ([#14725](#14725)) ([1ffa467](1ffa467)) * **gax-httpjson:** gracefully handle error details with unrecognized types ([#14612](#14612)) ([7295640](7295640)) * revise health metrics inflightBytes to become inflightBytesMax ([#14613](#14613)) ([44a12ef](44a12ef)) * **showcase:** update parent context retry tests for operation span in ITOtelTracing ([#14673](#14673)) ([312b5ea](312b5ea)) * **spanner:** increase max inbound message size from 256MB to 300MB ([#14551](#14551)) ([70c7439](70c7439)) * **spanner:** increase max inbound message size from 256MB to 300MB as ([70c7439](70c7439)) * **spanner:** prevent stream leaks and race conditions on result set close ([#14665](#14665)) ([013b49b](013b49b)) * **spanner:** support autoscaling config, edition, and positive capacity in cloud instance actions ([#14574](#14574)) ([004f0fd](004f0fd)) ### Performance Improvements * **bigquery:** defer blocking jobs.get RPC in QueryPageFetcher until getNextPage() ([#14650](#14650)) ([6183b63](6183b63)) ### Dependencies * Upgrade jackson-bom to v2.22.4 ([#14701](#14701)) ([09ce4c5](09ce4c5)) ### Documentation * **auth:** clarify quota project behavior on ImpersonatedCredentials ([#14597](#14597)) ([4e5a0d0](4e5a0d0)) * **pubsub:** Create a Cloud Pub/Sub sample for publishing with hedging ([#14623](#14623)) ([9de611f](9de611f)) * **pubsub:** regenerate README.md sample table ([#14643](#14643)) ([5394618](5394618)), refs [#14543](#14543) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Ensure when ResultSet is closed, we stop reading arrow stream as well