Add SRV DNS record support to HTTP Operator - #70960
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
fa9502b to
ff4e96a
Compare
0a99776 to
07f26a6
Compare
aaron-y-chen
left a comment
There was a problem hiding this comment.
Thanks for the PR 🙂
634914d to
09cda3c
Compare
omkar-foss
left a comment
There was a problem hiding this comment.
PR looks good to me overall. Newly added tests are looking well-covered, would be nice if someone could initiate the ci checks in this PR so we can see them run and pass.
PS: I'm not a maintainer, my review is non-binding. Just trying to help out :)
e959fc5 to
dd5042c
Compare
|
Found out the root cause CI failures (3/3 runs) targeting In Python 3.10(CI Env), Python 3.12 uses Added a This fixture re-registers the submodules: importlib.import_module("dns.resolver")
importlib.import_module("dns.asyncresolver")Drafted-by: Claude Code (Sonnet 5); reviewed by @gtxu |
| Use DNS SRV Lookup (optional) | ||
| Treat the Host field as a DNS SRV record name and resolve the target host/port at request time. | ||
|
|
||
| SRV Cache TTL (seconds) (optional) |
There was a problem hiding this comment.
I can understand your reasoning, but I don't think this is right for two reasons.
- DNS caching is a notoriously complex thing to deal with (do you cache poisitve, or NX/negative answers too)
- The practical benefit of caching is very limited, because this caching will apply to exactly one connection, which in all likely hood will just resolve one host then make connections to it.
There was a problem hiding this comment.
Hi @ashb , I agree with your feedback that we should drop the DNS caching.
One thing worth noting for callers using the multi-call async session pattern (HttpAsyncHook.session()) with SRV lookup enabled: without a cache, resolving the SRV record fresh on every request means the target host/port can change between calls. That effectively defeats aiohttp's pooled TCPConnector (which keys connections on (scheme, host, port)), so TCP keep-alive won't be as effective across those dynamic routes. Given the short-lived HttpHook/HttpAsyncHook instances in Airflow tasks, this trade-off is minor.
Removed cache from hooks and tests. Thank you for reviewing this PR!
2dcf401 to
d146a1f
Compare
d146a1f to
386999b
Compare
This pull request allows an HTTP connection resolve its target host and port from a DNS SRV record instead of a fixed host/port, so it can talk to services published via SRV-based service discovery(ie. AWS Service Discovery for ECS).
This changes introduces
dnspythonas an optional dependency to keep the core HTTP provider lightweight. To prevent DNS query flooding, a TTL cache mechanism is implemented.Implemented soft dependency via the
srvextra.Built RFC 2782 compliant failover (lowest priority tier filtering and random target selection).
Added concurrency-safe TTL caching (
srv_cache_ttl) usingasyncio.Lockto prevent cache stampedes in asynchronous contexts.Exposed
srv_lookupandsrv_cache_ttlconfiguration directly in the Airflow Connection UI via custom form widgets.closes: #9186
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Sonnet 5 following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.