-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Add DNSResolutionError for handling DNS resolution failures #360
base: main
Are you sure you want to change the base?
fix: Add DNSResolutionError for handling DNS resolution failures #360
Conversation
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
🤖 Devin AI EngineerOriginal prompt from Aaron:
I'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
@@ -59,3 +59,24 @@ class DefaultBackoffException(BaseBackoffException): | |||
|
|||
class RateLimitBackoffException(BaseBackoffException): | |||
pass | |||
|
|||
|
|||
class DNSResolutionError(BaseBackoffException): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where this exception class is raised. Can you help me understand this?
Does our code need to raise this exception in order for its handling to trigger? Or is it going to be picked up automatically somehow?
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
The DNSResolutionError is raised in http_client.py's _send method when we catch a requests.ConnectionError that contains DNS-related error messages. Here's the relevant code: try:
response = self._session.send(request, **request_kwargs)
except requests.ConnectionError as e:
if "Name or service not known" in str(e) or "nodename nor servname provided" in str(e):
assert request.url is not None, "Request URL cannot be None for DNS resolution error"
exc = DNSResolutionError(
url=request.url, request=request, response=e, error_message=str(e)
)
else:
exc = e This allows us to distinguish between actual DNS resolution failures (which should be retried) and invalid URLs (which are config errors and should fail immediately). The error handler will then interpret this exception and apply the appropriate retry policy. |
@@ -300,6 +301,16 @@ def _send( | |||
|
|||
try: | |||
response = self._session.send(request, **request_kwargs) | |||
except requests.ConnectionError as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we only catch InvalidURL
exceptions here? It looks like previously, these failures were raised and custom-handled as InvalidURL
exceptions.
We should catch here rather than because:
The key difference is:
Previously, both were treated as , but this meant we weren't retrying on temporary DNS failures. This change allows us to:
The error handler will then apply the appropriate retry policy based on the specific exception type. |
def test_dns_resolution_error_retry(): | ||
"""Test that DNS resolution errors are retried""" | ||
stream = StubHttpStreamWithErrorHandler() | ||
error_handler = stream.get_error_handler() | ||
request = requests.PreparedRequest() | ||
request.url = "https://example.com" | ||
dns_error = DNSResolutionError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test that simulates/mocks the error we expect the http api to raise? Specifically, I think we're expecting something like an invalid URL code, except with the noted error text about DNS.
Co-Authored-By: Aaron <AJ> Steers <[email protected]>
Description
This PR adds a new DNSResolutionError class to handle DNS resolution failures while maintaining security checks for invalid URLs. This addresses issue airbytehq/airbyte-internal-issues#11320.
Key changes:
Link to Devin run: https://app.devin.ai/sessions/6b54ed98f6e84226b1f5a7cb2d1a941d
Requested by: AJ