Repository navigation
fix(bigquery-jdbc): stop producer and release resources on ResultSet.close - #14712
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
3 participants
Ensure when ResultSet is closed, we stop reading arrow stream as well