github-actions[bot] commented on code in PR #69423:
URL: https://github.com/apache/doris/pull/69423#discussion_r4344955124
##########
be/src/service/http/action/stream_load.cpp:
##########
@@ -822,12 +822,17 @@ Status StreamLoadAction::_process_put(HttpRequest*
http_req,
#ifndef BE_TEST
// plan this load
- TNetworkAddress master_addr = _exec_env->cluster_info()->master_fe_addr;
int64_t stream_load_put_start_time = MonotonicNanos();
- RETURN_IF_ERROR(ThriftRpcHelper::rpc<FrontendServiceClient>(
- master_addr.hostname, master_addr.port,
+ RETURN_IF_ERROR(ThriftRpcHelper::rpc_fe_with_master_refresh(
+ [this]() { return _exec_env->cluster_info()->master_fe_addr; },
[&request, ctx](FrontendServiceConnection& client) {
+ ctx->put_result = {};
client->streamLoadPut(ctx->put_result, request);
+ },
+ [ctx]() { return ctx->put_result.status; },
+ [ctx]() -> const TNetworkAddress* {
+ return ctx->put_result.__isset.master_address ?
&ctx->put_result.master_address
Review Comment:
[P1] Match the planning redirect callback to its Thrift result type
`ctx->put_result` is a `TStreamLoadPutResult`, but that struct in
`FrontendService.thrift` still has only fields 1 and 3–9 and does not define
`master_address`. Consequently neither `put_result.__isset.master_address` nor
`put_result.master_address` exists in the generated C++ type, so this code and
the identical callback in `http_stream.cpp` fail the production BE build. Add
the compatible optional result field and populate it in the planning handler if
planning redirects are intended, or use a callback that does not access a
nonexistent member.
##########
fe/fe-core/src/main/java/org/apache/doris/common/DNSCache.java:
##########
@@ -60,6 +60,10 @@ public String get(String hostname) {
return cache.computeIfAbsent(hostname, this::resolveHostname);
}
+ public void invalidate(String hostname) {
+ cache.remove(hostname);
Review Comment:
[P2] Retain the FE DNS fallback when forcing a refresh
`Backend.invalidateLocalConnections()` calls this on every successful
recovery or process-epoch change, including heartbeat replay. If DNS briefly
fails while the backend is healthy at its previous IP, removing the entry
discards `resolveHostname()`'s fallback; the next `get()` caches its
empty-string result via `computeIfAbsent`, and `BackendServiceProxy.getProxy()`
then rejects every RPC in FQDN mode until the periodic DNS refresh (60
seconds), even after DNS recovers. A successful heartbeat can use the separate
cached heartbeat connection, so it does not rule out this scenario. Mark the
entry for refresh while preserving its known IP, as the BE cache now does. This
is the separate FE cache instance, rather than the BE path covered by the
existing review thread.
##########
be/src/util/dns_cache.cpp:
##########
@@ -102,7 +114,14 @@ Status DNSCache::get(const std::string& hostname,
std::string* ip) {
}
}
- // First access (or negative TTL expired): resolve and populate the cache.
+ // Claim the refresh before resolving so an invalidation arriving during
DNS
+ // resolution is retained for the next get(). Cache hits above still use
shared_lock.
+ {
+ std::unique_lock<std::shared_mutex> lock(mutex);
+ _refresh_pending.erase(hostname);
Review Comment:
[P2] Fence forced DNS refreshes against older background results
The forced-refresh path can now run `_update()` concurrently with the
background refresh for a cached hostname. If the background lookup captures old
IP A and blocks, a socket fault can trigger this path and successfully cache
replacement IP B. When the older lookup later fails with `EAI_AGAIN`,
`_resolve_hostname()` returns its captured A and `_update()` overwrites B with
it; `_refresh_pending` has already been cleared. The next `get_client()` then
sees A, discards the working B channel, and reconnects to the dead pod.
Serialize per-host refreshes or fence their results so a stale fallback cannot
overwrite a newer successful resolution.
--
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]
