fix: honor configured query retries and correct the retry debug event#730
Merged
Merged
Conversation
Collaborator
|
Just flagged this for final release 1.10.1 |
6933201 to
94dd8a0
Compare
The query-level retry in SQLServerConnectionManager.add_query had two defects: - A `credentials.retries > 3` guard silently ignored configured retry counts of 1-3 and fell back to a hardcoded limit of 2, so even the default `retries: 3` only attempted a query twice. Use `credentials.retries` directly, matching how open() drives connection retries. - The retry-debug event was built as `AdapterEventDebug(message=...)`, but the event field is `base_msg`; constructing it raised a protobuf ParseError on the first retryable error, so retries crashed instead of retrying. Use `base_msg=` as dbt-core's base add_query does. Add unit tests covering retry-until-success, the retries=3 regression, no-retry at retries=1, and non-retryable errors not being retried.
Drop the event-construction comment narrating the old message= defect and reduce the retry_limit comment to the one fact a reader needs: retries caps total execute attempts, so retries: 1 disables retry.
The three failure-path tests differed only in retries, side effect, expected exception and expected attempt count; collapse them into one parametrized test. Also drop the unused connection element from the test helper's return value.
94dd8a0 to
4a1e661
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes two defects in the query-level retry path in
SQLServerConnectionManager.add_query.1. The retry-debug event was constructed with a nonexistent field. On a retryable error, the code fired
AdapterEventDebug(message=...), but the event's field isbase_msg. Construction does not crash: dbt-common logs an "Unable to parse logging event dictionary" warning and emits the event with an empty message (verified by constructing the event against the pinned dbt-adapters/dbt-common in this repo's environment). The retry itself still ran, but the debug line that should report the error and remaining attempts was blank. This now usesbase_msg=, matching dbt-core's baseadd_query, which is where this method was originally copied from.2. Configured retry counts of 1-3 were silently ignored. The retry limit was computed as
credentials.retries if credentials.retries > 3 else retry_limit, whereretry_limitis a hardcoded 2. Any configuredretriesvalue of 1, 2, or 3 was discarded in favor of 2, so even the defaultretries: 3capped a query at two attempts. The configured value is now passed through directly.Semantics
For queries,
retriescaps the total number of execute attempts, matching the baseSQLConnectionManager.add_querycontract (attempt >= retry_limit):retries: 3means up to three attempts (two retries), andretries: 1means a single attempt with no retry. Note this differs from connection opening, whereretry_connectiontreats the same profile value as a retry count (retries: 3allows up to four connect attempts). That inconsistency exists upstream betweenadd_queryandretry_connectionin dbt-adapters; this PR standardizes query behavior on the baseadd_querysemantics and documents it in the code.Scope note
No in-tree caller currently passes
retryable_exceptionstoadd_query:execute()callsself.add_query(sql, auto_begin), and the backend retryable-exception tuples (get_pyodbc_retryable_exceptions/get_mssql_python_retryable_exceptions) are only wired intoretry_connectioninopen(). This PR therefore fixes the retry mechanism for callers that opt in, but does not by itself enable query retries on the standard execute path. Wiring the backend tuples intoexecute()is deliberately left as a follow-up, since auto-retrying arbitrary statements after e.g. a dropped connection can double-execute non-idempotent SQL and deserves its own discussion.Tests
Adds unit tests for
add_query's retry path:retries: 3attempts three times),retries: 1(single attempt, no retry), and non-retryable errors raising immediately with no retry.Verification