morningman commented on PR #63363:
URL: https://github.com/apache/doris/pull/63363#issuecomment-5084049517
## 1. Fix the condition-variable lost-wakeup in the destructor (Medium)
In `be/src/util/dns_cache.cpp`, the destructor sets `stop_refresh` and calls
`_cv.notify_all()`
**without holding `_cv_mutex`**:
```cpp
DNSCache::~DNSCache() {
stop_refresh = true; // not under _cv_mutex
_cv.notify_all();
...
}
```
The refresh thread waits with `_cv.wait_for(lk, 1min, [this]{ return
stop_refresh.load(); })`. Even
though `stop_refresh` is atomic, `std::condition_variable` requires the
predicate to be mutated while
holding the wait mutex; otherwise the notification can be lost if it lands
in the window after the
thread evaluates the predicate as `false` but before it actually blocks.
When that happens the thread
sleeps until the 1-minute timeout, so `~DNSCache()` stalls on `join()` for
up to ~60 s at BE
shutdown — defeating the whole reason for switching from `sleep_for` to a
condition variable.
**Fix** — set the flag under the lock, then notify:
```cpp
DNSCache::~DNSCache() {
{
std::lock_guard<std::mutex> lk(_cv_mutex);
stop_refresh = true;
}
_cv.notify_all();
if (refresh_thread.joinable()) {
refresh_thread.join();
}
}
```
## 2. Add a short negative-result cache to avoid blocking re-resolves after
eviction (Medium)
Before this PR, a host that had ever resolved stayed in the cache, so
`get()` was a fast read-lock
hit returning the (stale) IP instantly. After eviction the host is gone from
the cache, so **every**
subsequent `get()` falls through to `_update()` → `_resolve_hostname()` →
`hostname_to_ip()`, which
is a **blocking `getaddrinfo`** that can stall for seconds on a DNS timeout.
There is no negative
cache and no single-flight, so multiple caller threads asking for the same
dead host each block on
their own lookup. All callers hit this on the request path
(`brpc_client_cache.h`, `client_cache.cpp`,
`proto_util.h`, `peer_file_cache_reader.cpp`, `vtablet_writer.cpp`, and the
cloud paths).
In practice the impact is bounded — once a backend is dropped and the FE
stops routing to it, callers
stop asking — but during the transition this converts "fast stale-IP
returns" into "repeated
multi-second blocking resolves." Consider a small negative-result cache
(remember "unresolvable until
now + backoff", or keep a tombstone entry with a short retry interval) so an
evicted-but-still-requested
host returns an error cheaply instead of re-incurring a full DNS timeout on
every call.
## 3. Reconcile the two configuration defaults (Low)
With the shipped defaults `dns_cache_log_every_n_failures = 60` and
`dns_cache_max_consecutive_failures = 30`, a cached host is evicted at
failure 30 — before the
`failures % 60 == 0` periodic log can ever fire. So on the cached-IP path
only the first-failure log
is emitted (then the eviction log), and `dns_cache_log_every_n_failures`
effectively has no influence
under defaults, which is surprising. Either document this interaction or
default
`dns_cache_log_every_n_failures <= dns_cache_max_consecutive_failures` so at
least one periodic
progress log appears before eviction.
--
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]
