github-actions[bot] commented on code in PR #69423:
URL: https://github.com/apache/doris/pull/69423#discussion_r4344469191
##########
fe/fe-core/src/test/java/org/apache/doris/qe/SimpleSchedulerTest.java:
##########
@@ -75,6 +75,27 @@ private static Map<Long, Backend> genBackends() {
return map;
}
+ @Test
+ public void testRecentErrorsAccumulateBeforeBlacklistThreshold() {
+ int count = Config.do_add_backend_black_list_threshold_count;
Review Comment:
[P1] Preserve the long type when saving blacklist thresholds
Both `Config.do_add_backend_black_list_threshold_count` and
`Config.do_add_backend_black_list_threshold_seconds` are declared as `long` in
`fe-common/.../Config.java`. Assigning them to these `int` locals requires an
explicit narrowing conversion, so the new test fails Java test compilation with
a possible lossy conversion error. Save both values as `long` so FE unit tests
can compile and the original settings can be restored without truncation.
##########
be/src/util/thrift_rpc_helper.cpp:
##########
@@ -143,4 +145,60 @@ template Status
ThriftRpcHelper::rpc<TPaloBrokerServiceClient>(
const std::string& ip, const int32_t port,
std::function<void(ClientConnection<TPaloBrokerServiceClient>&)>
callback, int timeout_ms);
+Status ThriftRpcHelper::rpc_fe_with_master_refresh(
+ std::function<TNetworkAddress()> address_provider,
+ std::function<void(ClientConnection<FrontendServiceClient>&)> callback,
+ std::function<TStatus()> response_status,
+ std::function<const TNetworkAddress*()> master_hint, int timeout_ms) {
+ const bool enabled = _s_exec_env->cluster_info()->in_graceful_shutdown();
+ return _retry_fe_rpc(
+ address_provider,
+ [&](const TNetworkAddress& address) {
+ auto status = rpc<FrontendServiceClient>(address.hostname,
address.port, callback,
Review Comment:
[P2] Preserve live master refresh when graceful mode is disabled
The hostname/port overload wraps `address` in a constant provider, so the
existing transport retry can no longer observe a new master heartbeat.
Previously the transaction callers passed `master_addr_provider` directly: if
the old master's socket closed after a new-master heartbeat arrived, `rpc()`
retried the new master. With `enable_graceful_shutdown=false` (the default),
the outer loop returns immediately after retrying the old endpoint, causing
Stream Load and shared Routine Load transaction RPCs to fail during failovers
that previously recovered. Keep the live-provider retry when this mode is
disabled.
##########
be/src/util/brpc_client_cache.h:
##########
@@ -66,25 +66,43 @@ using StubMap = phmap::parallel_flat_hash_map<
std::allocator<std::pair<const std::string, StubEntry<T>>>, 8,
std::mutex>;
namespace doris {
+inline bool is_brpc_network_fault(int error) {
+ return error == EHOSTDOWN || error == EHOSTUNREACH || error == ENETUNREACH
||
+ error == ECONNREFUSED || error == ECONNRESET || error == ENOTCONN
|| error == EPIPE;
+}
+
+inline void on_brpc_network_fault(const std::shared_ptr<AtomicStatus>&
channel_status,
+ const std::string& hostname,
brpc::Controller* controller) {
+ auto status = Status::NetworkError("brpc network failure: {}",
controller->ErrorText());
+ channel_status->update(status);
+ // Retrying against the same cached IP after a pod replacement cannot
recover.
+ // Preserve the hostname-keyed stub map and normal DNS eviction / negative
TTL rules.
+ if (!hostname.empty() && !is_valid_ip(hostname)) {
+ auto* dns_cache = ExecEnv::GetInstance()->dns_cache();
+ // get_client also supports startup / test environments without a DNS
cache.
+ if (dns_cache != nullptr) {
+ dns_cache->invalidate(hostname);
Review Comment:
[P2] Keep the last known IP while refreshing DNS after socket errors
`ECONNRESET`/`EPIPE` can occur while the peer remains at the same IP. If DNS
is temporarily returning `EAI_AGAIN`, this unconditional invalidation deletes
the cached address that `DNSCache::_resolve_hostname()` intentionally uses as
fallback. The next `get_client()` then fails resolution, installs a
negative-cache entry, and refuses subsequent connections for
`dns_cache_negative_ttl_seconds` (60 seconds by default), even after DNS
recovers. This affects ordinary BRPC traffic with graceful mode disabled too.
Force a refresh while retaining the last known address on transient resolver
failures, and replace or evict it only with appropriate DNS evidence.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SimpleScheduler.java:
##########
@@ -384,10 +402,19 @@ public void run() {
LOG.info("remove backend {} from black list
because it does not exist", backendId);
} else {
BlackListInfo blackListInfo = entry.getValue();
- if (backend.isAlive() ||
blackListInfo.shouldBeRemoved()) {
+ if (blackListInfo.shouldBeRemoved()) {
+ // Backend has been in blacklist long enough,
remove it
+ iterator.remove();
+ LOG.info("remove backend {} from black list.
timeout reached,"
+ + " backend is alive: {}", backendId,
backend.isAlive());
+ } else if (backend.isAlive() &&
!blackListInfo.isBlacked()
Review Comment:
[P2] Update scheduler tests for the new blacklist retention rule
Fully blacklisted backends now remain excluded until
`stay_in_backend_black_list_threshold_seconds` expires (60 seconds by default),
but `SimpleSchedulerTest.testGetHostNormal` still expects availability 5
seconds after `setAlive(true)`, and `testGetHostAbnormal` expects `getHost()`
to succeed after 7 seconds. Both tests fully blacklist their backends and leave
that timeout at its default, so they fail under this new branch once test
compilation is fixed. Update those tests to cover retention and expiry with a
controlled timeout or clock.
--
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]
