Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a shared, thread-safe BigQuery Storage client for ARROW query results, allowing multiple queries to reuse a single client instance. It also configures the shared client's gRPC transport with keepalive pings to prevent idle connections from being dropped, and ensures the shared client is closed when the main BigQuery client is closed. The feedback suggests defensively handling the options parameter in create_channel to prevent a potential TypeError if None is passed.
| def create_channel(*args, options=(), **channel_kwargs): | ||
| options = list(options) + [ | ||
| ("grpc.keepalive_time_ms", _BQSTORAGE_KEEPALIVE_TIME_MS) | ||
| ] | ||
| return transport_class.create_channel(*args, options=options, **channel_kwargs) |
There was a problem hiding this comment.
If options is explicitly passed as None to create_channel, calling list(options) will raise a TypeError: 'NoneType' object is not iterable. Using options or [] ensures defensive handling and prevents potential runtime crashes.
| def create_channel(*args, options=(), **channel_kwargs): | |
| options = list(options) + [ | |
| ("grpc.keepalive_time_ms", _BQSTORAGE_KEEPALIVE_TIME_MS) | |
| ] | |
| return transport_class.create_channel(*args, options=options, **channel_kwargs) | |
| def create_channel(*args, options=None, **channel_kwargs): | |
| options = list(options or []) + [ | |
| ("grpc.keepalive_time_ms", _BQSTORAGE_KEEPALIVE_TIME_MS) | |
| ] | |
| return transport_class.create_channel(*args, options=options, **channel_kwargs) |
References
- When processing optional parameters with a helper function, prefer placing the null or None check within the function body rather than at the call site to simplify the calling code and improve encapsulation.
47c18a9 to
18c7c06
Compare
18c7c06 to
de75b51
Compare
ARROW query results created a new BigQueryReadClient for every read, even ones that never opened a stream, and to_arrow_iterable() never closed it, so each read left a gRPC channel open until garbage collection. The new client also ignored the BigQuery client's universe domain. Client now creates one BigQueryReadClient under a lock the first time an ARROW result needs a stream, with its credentials and universe domain, and reuses it. close() closes it, and a later query creates a new one. Results that fit in the first page never create it. A bqstorage_client passed to to_arrow_iterable() is used instead and never closed. Other BigQuery Storage reads keep creating their own clients. The shared client's gRPC channel sets keepalive (60s ping time, 60s ping timeout) through the generated transport's channel callable. gRPC stops pinging after two pings while ReadRows sends nothing, so longer pauses rely on ReadRowsStream resuming at the next unread row.
de75b51 to
3cba84a
Compare
Shares one
BigQueryReadClientperClientfor Arrow results, instead of creating a new one for every read and never closing it.Client.close().bqstorage_clientpassed by the caller instead, without closing it.grpc.keepalive_time_ms: 60000,grpc.keepalive_timeout_ms: 60000).