fix: honor configured query retries and stop retry-event crash#730
Draft
joshmarkovic wants to merge 1 commit into
Draft
fix: honor configured query retries and stop retry-event crash#730joshmarkovic wants to merge 1 commit into
joshmarkovic wants to merge 1 commit into
Conversation
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.
Collaborator
|
Just flagged this for final release 1.10.1 |
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 crashed instead of retrying. On a retryable error, the code fired
AdapterEventDebug(message=...), but the event field isbase_msg. Constructing the event withmessage=raises a protobufParseError, so the first retryable error crashed at event construction rather than triggering a retry. dbt-core's baseadd_queryusesbase_msg=; this matches it.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. So any configuredretriesvalue of 1, 2, or 3 was discarded in favor of 2, meaning even the defaultretries: 3only attempted a query twice. This now passescredentials.retriesdirectly, mirroringopen(), which already drives connection retries offcredentials.retries.Behavior change
With these fixes, query retries actually function (previously the first retryable error raised a
ParseError), and the configuredretriesvalue is honored. With the defaultretries: 3, a retryable query is now attempted up to 3 times rather than 2.Tests
Adds unit tests for
add_query's retry path:retries: 3regression (attempts the configured number of times),retries: 1,Verification
dbt/adapters/sqlserver/sqlserver_connections.pywas unchanged between the base this work started from and current master, so the change applies cleanly with no overlap against recent work.