github-actions[bot] commented on code in PR #63363:
URL: https://github.com/apache/doris/pull/63363#discussion_r3653768544
##########
be/src/util/dns_cache.cpp:
##########
@@ -67,26 +73,74 @@ 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 (!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};
Review Comment:
After eviction, every retry now misses the cache and performs synchronous
`getaddrinfo`, but this counter throttles only the extra `DNSCache` warning.
`hostname_to_ipv4/6` has already emitted an unconditional warning for the
failed lookup, and `BrpcClientCache` (plus the direct proto/cloud/writer/peer
callers) logs the non-OK status again. In the issue's hot retry scenario,
eviction therefore changes one background lookup per minute into blocking DNS
work and warnings per request, so the caller-side flood remains. Please keep
bounded per-host negative/backoff state (without returning the stale IP),
centralize/rate-limit the whole warning path, and test repeated normal-client
calls after eviction.
##########
be/src/util/dns_cache.cpp:
##########
@@ -97,25 +151,62 @@ Status DNSCache::_update(const std::string& hostname) {
cache[hostname] = real_ip;
LOG(INFO) << "update hostname " << hostname << "'s ip to " << real_ip;
}
+ 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() {
+ std::unordered_set<std::string> keys;
+ {
+ std::shared_lock<std::shared_mutex> lock(mutex);
+ std::transform(cache.begin(), cache.end(), std::inserter(keys,
keys.end()),
+ [](const auto& pair) { return pair.first; });
+ }
+ for (auto& key : keys) {
+ uint32_t failures = 0;
+ Status st = _update(key, &failures);
+ if (!st.ok()) {
+ // _update only returns an error when _resolve_hostname returns an
+ // empty string, which happens only if the hostname has never been
+ // successfully resolved (no cached IP to fall back to). Keys in
+ // the refresh loop come from `cache`, so they all have a prior IP;
+ // during DNS failure _resolve_hostname returns that cached IP and
+ // _update returns OK. This branch is therefore effectively dead
+ // under normal refresh semantics — the eviction logic below
handles
+ // the long-running failure case instead.
+ LOG(WARNING) << "Failed to update DNS cache for hostname " << key
<< ": "
+ << st.to_string();
+ }
+ // Evict hostnames that have failed to resolve for too long.
+ // This avoids two pathological symptoms after a backend is dropped
+ // from the cluster and its DNS record is removed:
+ // 1) be.WARNING gets flooded with `failed to get ip from host`.
+ // 2) brpc keeps re-using the stale IP from cache, producing
+ // `Fail to wait EPOLLOUT ... Connection timed out`.
+ int32_t threshold = config::dns_cache_max_consecutive_failures;
+ if (threshold > 0 && failures >= static_cast<uint32_t>(threshold)) {
Review Comment:
At `dns_cache_max_consecutive_failures=1`, this decision can erase a newer
successful generation. Start caller G on an absent key and block its successful
DNS lookup; let caller H resolve and insert the key; then let refresh snapshot
it, fail once, and return `failures == 1`. If G now completes, it clears
`failure_count` and publishes a fresh result before refresh reaches this check,
but refresh still uses the captured `1` and `_erase()` deletes G's success.
This is distinct from the existing stale-fallback reinsert at line 152: fixing
fallback provenance does not fence this later erase. Please re-check the live
counter/generation under the same lock as erase and add a barrier-controlled
test for this handoff.
--
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]
