gtxu commented on code in PR #70960:
URL: https://github.com/apache/airflow/pull/70960#discussion_r3771235515
##########
providers/http/docs/connections/http.rst:
##########
@@ -47,12 +47,22 @@ Password (optional)
Host (optional)
Specify the entire url or the base of the url for the service.
+ If "Use DNS SRV Lookup" is enabled, specify the DNS SRV record name instead
+ (e.g. ``_http._tcp.example.com``) - Note the actual host and port are
resolved from DNS at
+ request time and any value set in the Port field is ignored.
+
Port (optional)
- Specify a port number if applicable.
+ Specify a port number if applicable. Ignored when SRV lookup is enabled.
Schema (optional)
Specify the service type etc: http/https.
+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)
Review 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
