github-actions[bot] commented on code in PR #63363:
URL: https://github.com/apache/doris/pull/63363#discussion_r3670698673
##########
be/src/util/dns_cache.cpp:
##########
@@ -26,35 +31,68 @@ DNSCache::DNSCache() {
refresh_thread = std::thread(&DNSCache::_refresh_cache, this);
}
+DNSCache::DNSCache(Resolver resolver) : _resolver(std::move(resolver)) {}
+
DNSCache::~DNSCache() {
- stop_refresh = true;
+ {
+ std::lock_guard<std::mutex> lk(_cv_mutex);
+ stop_refresh = true;
+ }
+ _cv.notify_all();
if (refresh_thread.joinable()) {
refresh_thread.join();
}
}
Status DNSCache::get(const std::string& hostname, std::string* ip) {
+ bool expired_negative = false;
{
std::shared_lock<std::shared_mutex> lock(mutex);
auto it = cache.find(hostname);
if (it != cache.end()) {
*ip = it->second;
return Status::OK();
}
+ // Return a cheap error for recently-evicted hosts to avoid repeated
+ // blocking getaddrinfo calls during the transition after a backend is
+ // dropped. The entry expires after dns_cache_negative_ttl_seconds;
+ // once expired the next caller falls through and retries the resolve.
+ auto neg_it = _negative_cache.find(hostname);
+ if (neg_it != _negative_cache.end()) {
+ if (std::chrono::steady_clock::now() < neg_it->second) {
Review Comment:
This ignores the live mutable setting once a tombstone exists. For example,
evict with `dns_cache_negative_ttl_seconds=3600`, then set it to `0` after DNS
recovers: this stored deadline still makes every `get()` fail for nearly the
original hour, although the config contract says `<= 0` disables the negative
cache. A decrease has the same stale-deadline problem. Please derive effective
expiry from the current TTL (for example by storing eviction time), or
clear/shorten existing tombstones on update, and test positive-to-zero plus
large-to-small transitions.
##########
be/src/util/dns_cache.cpp:
##########
@@ -67,55 +105,190 @@ std::string DNSCache::_resolve_hostname(const
std::string& hostname) {
// Try to resolve hostname
std::string resolved_ip;
- Status status = hostname_to_ip(hostname, resolved_ip,
BackendOptions::is_bind_ipv6());
+ Status status = _resolver
+ ? _resolver(hostname, resolved_ip,
BackendOptions::is_bind_ipv6())
+ : hostname_to_ip(hostname, resolved_ip,
BackendOptions::is_bind_ipv6());
if (!status.ok() || resolved_ip.empty()) {
- // Resolution failed
+ if (is_fresh) *is_fresh = false;
if (!cached_ip.empty()) {
- LOG(WARNING) << "Failed to resolve hostname " << hostname
- << ", use cached ip: " << cached_ip;
+ // Only track failure counts for hosts that are currently in the
cache.
+ // Hosts that were never cached or have already been evicted are
not
+ // tracked, which prevents unbounded growth of failure_count.
+ uint32_t failures = 0;
+ {
+ std::unique_lock<std::shared_mutex> lock(mutex);
+ // Re-check that the host is still cached under the
unique_lock:
+ // it may have been evicted by the refresh thread between our
+ // earlier shared_lock read of cached_ip and now
(hostname_to_ip
+ // can block for seconds on DNS timeout, widening the window).
+ // Skipping the bump here preserves keys(failure_count) ⊆
keys(cache).
+ if (cache.find(hostname) != cache.end()) {
+ failures = ++failure_count[hostname];
+ }
+ }
+ // Throttle the log: only every N failures or the first failure.
+ if (failures > 0) {
+ int32_t every_n = std::max(1,
config::dns_cache_log_every_n_failures);
+ if (failures == 1 || failures % static_cast<uint32_t>(every_n)
== 0) {
+ LOG(WARNING) << "Failed to resolve hostname " << hostname
+ << " (consecutive failures: " << failures
+ << "), use cached ip: " << cached_ip;
+ }
+ }
return cached_ip;
} else {
- LOG(WARNING) << "Failed to resolve hostname " << hostname << ", no
cached ip available";
+ // Throttle to avoid flooding be.WARNING when callers repeatedly
+ // query an evicted or never-resolvable hostname. This branch
+ // deliberately does not maintain a per-hostname counter (that
+ // would break the keys(failure_count) ⊆ keys(cache) invariant),
+ // so the throttle is a coarse global rate limit shared across
+ // all hostnames hitting this code path.
+ static std::atomic<uint64_t> no_cache_warn_counter {0};
+ uint64_t n = no_cache_warn_counter.fetch_add(1,
std::memory_order_relaxed) + 1;
+ int32_t every_n = std::max(1,
config::dns_cache_log_every_n_failures);
+ if (n == 1 || n % static_cast<uint64_t>(every_n) == 0) {
+ LOG(WARNING) << "Failed to resolve hostname " << hostname
+ << ", no cached ip available";
+ }
return "";
}
}
+ // Resolution succeeded - clear failure counter for this hostname.
+ if (is_fresh) *is_fresh = true;
+ {
+ std::unique_lock<std::shared_mutex> lock(mutex);
+ failure_count.erase(hostname);
+ }
return resolved_ip;
}
-Status DNSCache::_update(const std::string& hostname) {
- std::string real_ip = _resolve_hostname(hostname);
+void DNSCache::_erase(const std::string& hostname) {
+ std::unique_lock<std::shared_mutex> lock(mutex);
+ cache.erase(hostname);
+ failure_count.erase(hostname);
+ int32_t ttl = config::dns_cache_negative_ttl_seconds;
+ if (ttl > 0) {
+ _negative_cache[hostname] =
+ std::chrono::steady_clock::now() + std::chrono::seconds(ttl);
+ }
+}
+
+bool DNSCache::_erase_if_still_failing(const std::string& hostname, uint32_t
threshold) {
+ std::unique_lock<std::shared_mutex> lock(mutex);
+ auto fc_it = failure_count.find(hostname);
+ if (fc_it == failure_count.end() || fc_it->second < threshold) {
+ // A concurrent successful resolution cleared or reset the counter
between
+ // _update() returning and this call — do not erase a now-healthy
entry.
+ return false;
+ }
+ cache.erase(hostname);
+ failure_count.erase(hostname);
+ int32_t ttl = config::dns_cache_negative_ttl_seconds;
+ if (ttl > 0) {
+ _negative_cache[hostname] =
+ std::chrono::steady_clock::now() + std::chrono::seconds(ttl);
+ }
+ return true;
+}
+
+Status DNSCache::_update(const std::string& hostname, uint32_t* out_failures,
std::string* out_ip) {
+ bool is_fresh = false;
+ std::string real_ip = _resolve_hostname(hostname, &is_fresh);
if (real_ip.empty()) {
+ if (out_failures) *out_failures = 0;
+ if (out_ip) out_ip->clear();
return Status::InternalError("Failed to resolve hostname {} and no
cached ip available",
hostname);
}
std::unique_lock<std::shared_mutex> lock(mutex);
+ // _resolve_hostname may have captured a stale cached_ip before a
concurrent
+ // eviction completed. If the host is now in the negative cache we must
not
+ // reinsert the stale IP: that would silently undo the eviction and clear
the
+ // tombstone, defeating the whole purpose of eviction. Only a fresh DNS
+ // result (is_fresh == true, meaning DNS actually resolved) may override an
+ // eviction — which indicates the backend is genuinely back.
+ if (!is_fresh && _negative_cache.count(hostname)) {
+ if (out_failures) *out_failures = 0;
+ if (out_ip) out_ip->clear();
+ return Status::InternalError(
+ "Hostname {} was concurrently evicted; stale-fallback not
reinserted", hostname);
+ }
auto it = cache.find(hostname);
if (it == cache.end() || it->second != real_ip) {
cache[hostname] = real_ip;
LOG(INFO) << "update hostname " << hostname << "'s ip to " << real_ip;
}
+ // DNS resolved successfully — remove any negative cache tombstone so
+ // subsequent get() calls go straight to the main cache.
+ _negative_cache.erase(hostname);
+ if (out_ip) *out_ip = real_ip;
+ // Read failure_count under the same lock we already hold, so _refresh_once
+ // does not need a second lock acquisition to decide on eviction.
+ if (out_failures) {
+ auto fc_it = failure_count.find(hostname);
+ *out_failures = fc_it != failure_count.end() ? fc_it->second : 0;
+ }
return Status::OK();
}
+void DNSCache::_refresh_once() {
+ // Purge expired negative-cache entries to prevent unbounded map growth.
+ {
+ std::unique_lock<std::shared_mutex> lock(mutex);
+ auto now = std::chrono::steady_clock::now();
+ for (auto it = _negative_cache.begin(); it != _negative_cache.end();) {
+ it = (now >= it->second) ? _negative_cache.erase(it) :
std::next(it);
Review Comment:
This cleanup removes the only marker that lets `get()` recognize a failed
retry and re-arm the backoff. With the defaults, eviction creates a 60-second
tombstone; the next refresh starts after another 60-second wait, erases it
here, and does not retry it because the hostname is no longer in `cache`. If
DNS is still down, the next request sees no tombstone, leaves
`expired_negative=false`, and skips the re-arm block—so every later serial
request again performs synchronous DNS indefinitely. Please preserve/transition
the per-host state until one caller claims the retry, and add an expire →
refresh cleanup → repeated failed `get()` test.
##########
be/src/util/dns_cache.cpp:
##########
@@ -26,35 +31,68 @@ DNSCache::DNSCache() {
refresh_thread = std::thread(&DNSCache::_refresh_cache, this);
}
+DNSCache::DNSCache(Resolver resolver) : _resolver(std::move(resolver)) {}
+
DNSCache::~DNSCache() {
- stop_refresh = true;
+ {
+ std::lock_guard<std::mutex> lk(_cv_mutex);
+ stop_refresh = true;
+ }
+ _cv.notify_all();
if (refresh_thread.joinable()) {
refresh_thread.join();
}
}
Status DNSCache::get(const std::string& hostname, std::string* ip) {
+ bool expired_negative = false;
{
std::shared_lock<std::shared_mutex> lock(mutex);
auto it = cache.find(hostname);
if (it != cache.end()) {
*ip = it->second;
return Status::OK();
}
+ // Return a cheap error for recently-evicted hosts to avoid repeated
+ // blocking getaddrinfo calls during the transition after a backend is
+ // dropped. The entry expires after dns_cache_negative_ttl_seconds;
+ // once expired the next caller falls through and retries the resolve.
+ auto neg_it = _negative_cache.find(hostname);
+ if (neg_it != _negative_cache.end()) {
+ if (std::chrono::steady_clock::now() < neg_it->second) {
+ return Status::InternalError(
+ "Hostname {} is in negative DNS cache (recently
evicted), skipping resolve",
+ hostname);
+ }
+ expired_negative = true; // TTL passed; allow one retry
Review Comment:
The expired entry is not claimed by any caller: every request can hold this
shared lock, observe the same deadline, set its own `expired_negative`, and
reach `_update()` before the first failure re-arms the map. In a retry burst,
that makes all N request threads block in `getaddrinfo` and emit resolver
errors at the TTL boundary, even though the new path claims one attempt per
host per TTL. This is distinct from the earlier no-backoff thread: it is a race
in the newly added expiry handoff. Please atomically mark one per-host retry as
in flight (without holding the lock during DNS), keep other callers on the
cheap-error path, and add a barrier-controlled test that proves one resolver
call for simultaneous expired-cache requests.
##########
be/src/util/dns_cache.cpp:
##########
@@ -67,55 +105,190 @@ std::string DNSCache::_resolve_hostname(const
std::string& hostname) {
// Try to resolve hostname
std::string resolved_ip;
- Status status = hostname_to_ip(hostname, resolved_ip,
BackendOptions::is_bind_ipv6());
+ Status status = _resolver
+ ? _resolver(hostname, resolved_ip,
BackendOptions::is_bind_ipv6())
+ : hostname_to_ip(hostname, resolved_ip,
BackendOptions::is_bind_ipv6());
if (!status.ok() || resolved_ip.empty()) {
- // Resolution failed
+ if (is_fresh) *is_fresh = false;
if (!cached_ip.empty()) {
- LOG(WARNING) << "Failed to resolve hostname " << hostname
- << ", use cached ip: " << cached_ip;
+ // Only track failure counts for hosts that are currently in the
cache.
+ // Hosts that were never cached or have already been evicted are
not
+ // tracked, which prevents unbounded growth of failure_count.
+ uint32_t failures = 0;
+ {
+ std::unique_lock<std::shared_mutex> lock(mutex);
+ // Re-check that the host is still cached under the
unique_lock:
+ // it may have been evicted by the refresh thread between our
+ // earlier shared_lock read of cached_ip and now
(hostname_to_ip
+ // can block for seconds on DNS timeout, widening the window).
+ // Skipping the bump here preserves keys(failure_count) ⊆
keys(cache).
+ if (cache.find(hostname) != cache.end()) {
+ failures = ++failure_count[hostname];
+ }
+ }
+ // Throttle the log: only every N failures or the first failure.
+ if (failures > 0) {
+ int32_t every_n = std::max(1,
config::dns_cache_log_every_n_failures);
+ if (failures == 1 || failures % static_cast<uint32_t>(every_n)
== 0) {
+ LOG(WARNING) << "Failed to resolve hostname " << hostname
+ << " (consecutive failures: " << failures
+ << "), use cached ip: " << cached_ip;
+ }
+ }
return cached_ip;
} else {
- LOG(WARNING) << "Failed to resolve hostname " << hostname << ", no
cached ip available";
+ // Throttle to avoid flooding be.WARNING when callers repeatedly
+ // query an evicted or never-resolvable hostname. This branch
+ // deliberately does not maintain a per-hostname counter (that
+ // would break the keys(failure_count) ⊆ keys(cache) invariant),
+ // so the throttle is a coarse global rate limit shared across
+ // all hostnames hitting this code path.
+ static std::atomic<uint64_t> no_cache_warn_counter {0};
+ uint64_t n = no_cache_warn_counter.fetch_add(1,
std::memory_order_relaxed) + 1;
+ int32_t every_n = std::max(1,
config::dns_cache_log_every_n_failures);
+ if (n == 1 || n % static_cast<uint64_t>(every_n) == 0) {
+ LOG(WARNING) << "Failed to resolve hostname " << hostname
+ << ", no cached ip available";
+ }
return "";
}
}
+ // Resolution succeeded - clear failure counter for this hostname.
+ if (is_fresh) *is_fresh = true;
+ {
+ std::unique_lock<std::shared_mutex> lock(mutex);
+ failure_count.erase(hostname);
+ }
return resolved_ip;
}
-Status DNSCache::_update(const std::string& hostname) {
- std::string real_ip = _resolve_hostname(hostname);
+void DNSCache::_erase(const std::string& hostname) {
+ std::unique_lock<std::shared_mutex> lock(mutex);
+ cache.erase(hostname);
+ failure_count.erase(hostname);
+ int32_t ttl = config::dns_cache_negative_ttl_seconds;
+ if (ttl > 0) {
+ _negative_cache[hostname] =
+ std::chrono::steady_clock::now() + std::chrono::seconds(ttl);
+ }
+}
+
+bool DNSCache::_erase_if_still_failing(const std::string& hostname, uint32_t
threshold) {
+ std::unique_lock<std::shared_mutex> lock(mutex);
+ auto fc_it = failure_count.find(hostname);
+ if (fc_it == failure_count.end() || fc_it->second < threshold) {
+ // A concurrent successful resolution cleared or reset the counter
between
+ // _update() returning and this call — do not erase a now-healthy
entry.
+ return false;
+ }
+ cache.erase(hostname);
Review Comment:
Eviction makes a DNS error a normal outcome for a hostname that previously
returned its cached IP, so downstream error handling now matters. In
`CloudWarmUpManager::_recycle_cache()` (`cloud_warm_up_manager.cpp:961-985`),
the first such replica takes `return` at line 971 and skips every later
replica; the sibling rowset warm-up loop correctly records the failure and
`continue`s. The `CloudTablet` callers do not retry this best-effort recycle
request, so healthy later replicas keep obsolete cache entries. Please make
this caller continue per replica (and audit the other new error consumers),
with a multi-replica test where a negative-cached host precedes a reachable
peer.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
