feat(bigquery): make BigQuery AutoCloseable with default no-op close method

[jinseopkim0]

Extend AutoCloseable on BigQuery with a default no-op close() method to allow deterministic resource management without breaking backwards compatibility.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request updates the BigQuery interface to extend AutoCloseable and introduces a default close() method. The BigQueryImpl class implements this method to safely close the underlying BigQueryReadClient using a lock, and unit tests are added to verify its behavior, idempotency, and compatibility with try-with-resources. There are no review comments, and I have no feedback to provide.

[gemini-code-assist]

Code Review

This pull request updates the BigQuery interface to extend AutoCloseable and implements the close() method in BigQueryImpl to release the underlying BigQueryReadClient resources, along with corresponding unit tests. Feedback suggests refactoring the close() implementation to perform the blocking client close operation outside of the locked block, thereby reducing lock contention and avoiding potential deadlocks.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request updates the BigQuery interface to extend AutoCloseable and implements the close() method in BigQueryImpl to clean up the underlying BigQueryReadClient. It also adds corresponding unit tests for idempotency and try-with-resources. The review feedback highlights a potential race condition where calling close() does not prevent subsequent operations from lazily recreating and leaking a new BigQueryReadClient instance, and suggests introducing a closed state flag to prevent this.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request updates the BigQuery interface to extend AutoCloseable and implements the close() method in BigQueryImpl to release background resources, specifically the underlying BigQueryReadClient. It also adds corresponding unit tests to verify the behavior, idempotency, and integration with try-with-resources. There are no review comments, and the implementation looks solid with no additional feedback to provide.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request updates the BigQuery interface to extend AutoCloseable and implements the close() method in BigQueryImpl to release cached BigQueryReadClient instances, along with comprehensive unit tests. The feedback recommends declaring the new 'closed' flag as transient and volatile to ensure thread safety and correct behavior during serialization.

[jinseopkim0]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request updates the BigQuery interface to extend AutoCloseable and implements the close() method in BigQueryImpl to properly release underlying background resources, specifically closing all cached BigQueryReadClient instances. It also introduces state checks to throw an IllegalStateException if clients are requested after the service has been closed, and adds comprehensive unit tests to verify this behavior. No review comments were provided, and the implementation looks solid with no additional feedback.

[lqiu96]

can you double check this: I think we only need to have a mutex on the closed var (e.g. mutex to set close = true).

Afterwards, we can begin the orderly shutdown of clients as new regions won't be able to create new bqReadClients.

I think the thing that comes to mind, is that most managed resources in the clients will invoke close() which invokes shutdown(). shutdown does not immediately force all resources to shutdown (any in flight processes will continue, but new requests will not be closed)

[jinseopkim0]

Thank you for the feedback, I agree. I've updated the code to narrow the mutex strictly to setting closed = true and initiate orderly shutdown of bqReadClients directly.

[lqiu96]

LGTM. Before merging, can you double check that this doesn't incur any weird or edge case behavioral breaking change?

I think source-wise we should be fine given that we cover the interface case with a default close method.