fix(bigquery): scope SSLError non-retryable to streaming inserts - #18202
fix(bigquery): scope SSLError non-retryable to streaming inserts#18202ayam04 wants to merge 4 commits into
Conversation
Sorry, something went wrong.
googleapis#17489 made SSLError globally non-retryable, but _should_retry is the base predicate for every retry surface: a single transient TLS reset (SSLEOFError) now fails jobs.get / result() polling outright, where 3.40.1 retried it as a ConnectionError subclass. Remove SSLError from the global non-retryable set and keep the googleapis#17489 carve-out only on the insertAll path via a scoped predicate (INSERT_ROWS_DEFAULT_RETRY): malformed streaming payloads still fail fast, while job polling keeps retrying transient transport resets. Fixes googleapis#18178
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request restricts the SSLError retry carve-out to the streaming-insert path, restoring global retries for transient SSLErrors to prevent polling failures. The reviewer correctly identified that the standard insert_rows method must also be updated to use INSERT_ROWS_DEFAULT_RETRY as its default parameter to prevent bypassing the new retry behavior during delegation.
Sorry, something went wrong.
| ignore_unknown_values: Optional[bool] = None, | ||
| template_suffix: Optional[str] = None, | ||
| retry: retries.Retry = DEFAULT_RETRY, | ||
| retry: retries.Retry = INSERT_ROWS_DEFAULT_RETRY, |
There was a problem hiding this comment.
The insert_rows method (defined elsewhere in this file) also has retry: retries.Retry = DEFAULT_RETRY as its default parameter value and delegates to insert_rows_json by passing retry=retry. Because of this, calling client.insert_rows(...) without a retry argument will explicitly pass DEFAULT_RETRY to insert_rows_json, bypassing the new INSERT_ROWS_DEFAULT_RETRY default. Please update the default value of the retry parameter in insert_rows to INSERT_ROWS_DEFAULT_RETRY as well to ensure the SSLError carve-out is correctly applied to the standard insert_rows path.
Sorry, something went wrong.
|
Thanks for the review. I verified this against the code before responding (per AGENTS.md): insert_rows (client.py:3808) has no explicit retry parameter — its signature is (self, table, rows, selected_fields=None, **kwargs), and it delegates via return self.insert_rows_json(table, json_rows, **kwargs) (client.py:3882). So when a caller invokes insert_rows without a retry argument, nothing is passed in kwargs, and insert_rows_json applies its new INSERT_ROWS_DEFAULT_RETRY default — the SSLError carve-out is correctly in effect on that path. The only bypass would be a caller explicitly passing retry=DEFAULT_RETRY to either method, which is their deliberate choice to override the default (same as today with any retry customization). I've added a unit test to lock this in: test_client_retry.py now asserts insert_rows_json uses the scoped predicate by default and insert_rows inherits it through delegation. |
Sorry, something went wrong.
|
Hi @plamut @HemangChothani — this fixes #18178: scoping the SSLError non-retryable carve-out from #17489 to the streaming-insert path only, restoring transient TLS reset retries for jobs.get / result() polling (production evidence in the issue: 49 polling failures across ~20 DAGs in ~19h on 3.42.2, zero on 3.40.1). Summary:
CLA signed, all checks green except the multi-approvers gate. Could one of you take a look? |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Fixes #18178
Problem
#17489 made requests.exceptions.SSLError globally non-retryable by adding it to _UNSTRUCTURED_NON_RETRYABLE_TYPES, but _should_retry is the base predicate for every retry surface in the client. A single transient TLS reset (e.g. SSLEOFError/UNEXPECTED_EOF_WHILE_READING on a pooled-connection handshake) now fails jobs.get / result() polling outright, where 3.40.1 retried it as the ConnectionError subclass requests deliberately makes it. Production evidence in the issue: 49 polling failures across ~20 DAGs in ~19 h after upgrading to 3.42.2, zero on 3.40.1.
Change
Tests
Validation