Ryan19929 commented on code in PR #68740:
URL: https://github.com/apache/doris/pull/68740#discussion_r4253915674
##########
be/src/storage/storage_engine.cpp:
##########
@@ -495,9 +495,15 @@ int64_t StorageEngine::get_file_or_directory_size(const
std::string& file_path)
void StorageEngine::_start_disk_stat_monitor() {
for (auto& it : _store_map) {
+ // Include IO-error handling: persisting the broken path can also
stall.
+ DiskHealthCheckWatchdog::ScopedCheck
check(_disk_health_check_watchdog);
Review Comment:
[P2] Document the semantic change and the restart behavior after a hang
With the watchdog enabled, a single hung disk terminates the whole BE
regardless of `max_percentage_of_error_disk` / `ignore_broken_disk`.
Unlike EIO, a hang never reaches `add_broken_path()`, so the path is not
persisted to `broken_storage_path`. After a restart, the startup
`check_datapath_rw()` hangs on the same disk and exits with 74 again, so an
auto-restarting BE crash-loops until the operator manually adds the path to
`broken_storage_path`.
That is a reasonable trade-off for a fail-fast option, but it should be in
the release note and the config docs together with the recovery step.
##########
be/src/common/config.cpp:
##########
@@ -376,6 +376,12 @@ DEFINE_mInt32(cache_periodic_prune_stale_sweep_sec, "60");
DEFINE_mInt32(tablet_lookup_cache_stale_sweep_time_sec, "30");
DEFINE_mInt32(point_query_row_cache_stale_sweep_time_sec, "300");
DEFINE_mInt32(disk_stat_monitor_interval, "5");
+// Maximum time for one startup disk/spill check, local-engine disk health
check,
Review Comment:
[P2] The probe includes fsync, so a small timeout can terminate a healthy BE
`read_write_test_file()` ends with `LocalFileWriter::close()`, which (with
the default `sync_file_on_close=true`) runs `fdatasync` on the test file and
`sync_dir` on its parent. On a healthy disk under heavy compaction/load write
pressure (e.g. ext4 `data=ordered`, where an fsync waits for the journal
commit), one probe can take seconds to tens of seconds.
If an operator sets this close to `disk_stat_monitor_interval` (5–10 s),
healthy BEs get `_exit(74)`. Write pressure is often cluster-wide, so several
BEs can exit at about the same time, which is worse than the stall this guards
against. The final supervised phase also waits for `_store_lock`, which
`get_all_data_dir_info()` holds across `update_capacity()` (statvfs) for every
disk.
Please state in this comment and in the config docs that the probe includes
file + directory fsync, and recommend a conservative minimum (e.g. >= 60 s), or
enforce a lower bound in the validator.
##########
be/src/storage/storage_engine.cpp:
##########
@@ -495,9 +495,15 @@ int64_t StorageEngine::get_file_or_directory_size(const
std::string& file_path)
void StorageEngine::_start_disk_stat_monitor() {
for (auto& it : _store_map) {
+ // Include IO-error handling: persisting the broken path can also
stall.
+ DiskHealthCheckWatchdog::ScopedCheck
check(_disk_health_check_watchdog);
it.second->health_check();
}
+ // These steps acquire _store_lock and may log a fatal disk failure. Keep
+ // supervising them so a stalled report or fatal logger cannot stop probing
+ // forever after the last per-disk check has completed.
+ DiskHealthCheckWatchdog::ScopedCheck check(_disk_health_check_watchdog);
Review Comment:
[P3] This scope also covers the `LOG(FATAL)` path
`_exit_if_too_many_disks_are_failed()` may `LOG(FATAL)`. If the failure
signal handler (stack symbolization and core dump) takes longer than the
timeout, the intended SIGABRT + core is preempted by a silent `_exit(74)`, and
the diagnostics are lost. The process exits either way, but consider ending the
scope before the fatal path, or documenting this.
##########
be/src/service/doris_main.cpp:
##########
@@ -505,12 +515,14 @@ int main(int argc, char** argv) {
}
if (paths.empty()) {
+ doris::DiskHealthCheckWatchdog::ScopedCheck
check(startup_disk_watchdog);
Review Comment:
[P3] This scope supervises `LOG` + `exit()`, not a disk check
The same applies at `:539`. Because `exit()` does not unwind locals, the
scope stays armed through `atexit` handlers and static destructors, so a slow
teardown changes the exit code from 255 to 74. A one-line comment on the intent
would help future readers.
##########
be/src/storage/disk_health_check_watchdog.cpp:
##########
@@ -0,0 +1,128 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "storage/disk_health_check_watchdog.h"
+
+#include <unistd.h>
+
+#include <ctime>
+#include <limits>
+
+#include "common/check.h"
+#include "common/status.h"
+#include "util/time.h"
+
+namespace doris {
+
+static_assert(std::atomic<int64_t>::is_always_lock_free);
+static_assert(std::atomic<bool>::is_always_lock_free);
+
+DiskHealthCheckWatchdog::~DiskHealthCheckWatchdog() {
+ stop();
+}
+
+Status DiskHealthCheckWatchdog::start(std::chrono::milliseconds timeout) {
+ // NOLINTNEXTLINE(readability-inconsistent-ifelse-braces): DORIS_CHECK
expands to if/else.
+ DORIS_CHECK(!_started && !_stopped.load(std::memory_order_acquire));
+ if (timeout.count() < 0 || timeout.count() >
std::numeric_limits<int32_t>::max() * 1000LL) {
+ return Status::InvalidArgument("Invalid disk health check timeout: {}
ms", timeout.count());
+ }
+ _timeout_ms = timeout.count();
+ if (_timeout_ms == 0) {
+ return Status::OK();
+ }
+ // Unlike ordinary Doris workers, this thread must work before ExecEnv is
+ // initialized and cannot depend on ThreadMgr locks or memory-tracker
setup.
+ // Its callback neither allocates nor logs and needs no Doris thread
context.
+ int err = pthread_create(
+ &_thread, nullptr,
+ [](void* arg) -> void* {
+ static_cast<DiskHealthCheckWatchdog*>(arg)->_run();
+ return nullptr;
+ },
+ this);
+ if (err != 0) {
+ _timeout_ms = 0;
+ return Status::RuntimeError("Cannot start disk health check watchdog,
pthread error {}",
+ err);
+ }
+ _started = true;
+ return Status::OK();
+}
+
+void DiskHealthCheckWatchdog::stop() {
+ if (_started) {
+ _stopped.store(true, std::memory_order_release);
+ int err = pthread_join(_thread, nullptr);
+ DORIS_CHECK_EQ(err, 0);
+ _started = false;
+ }
+}
+
+DiskHealthCheckWatchdog::ScopedCheck::ScopedCheck(DiskHealthCheckWatchdog&
watchdog)
+ : _watchdog(watchdog), _deadline(watchdog._begin_check()) {}
+
+DiskHealthCheckWatchdog::ScopedCheck::~ScopedCheck() {
+ _watchdog._end_check(_deadline);
+}
+
+int64_t DiskHealthCheckWatchdog::_begin_check() {
+ if (_timeout_ms == 0) {
+ return 0;
+ }
+ const int64_t deadline = MonotonicMillis() + _timeout_ms;
+ int64_t idle = 0;
+ const bool armed = _deadline.compare_exchange_strong(idle, deadline,
std::memory_order_acq_rel);
+ if (!armed && idle == kTimedOut) {
+ // The poller may have claimed the previous check and then been
+ // descheduled before _exit(). Do not enter logging/exception handling
+ // when this producer completes and advances to its next check.
+ _exit(kTimeoutExitCode);
+ }
+ // Checks from this producer must not overlap.
+ // NOLINTNEXTLINE(readability-inconsistent-ifelse-braces): DORIS_CHECK
expands to if/else.
+ DORIS_CHECK(armed);
+ return deadline;
+}
+
+void DiskHealthCheckWatchdog::_end_check(int64_t deadline) {
+ if (deadline != 0) {
+ // Never clear the terminal state after the watchdog has claimed a
timeout.
+ _deadline.compare_exchange_strong(deadline, 0,
std::memory_order_acq_rel);
+ }
+}
+
+bool DiskHealthCheckWatchdog::_claim_timeout(int64_t deadline, int64_t now) {
+ return deadline > 0 && now >= deadline &&
+ _deadline.compare_exchange_strong(deadline, kTimedOut,
std::memory_order_acq_rel);
+}
+
+void DiskHealthCheckWatchdog::_run() {
+ while (!_stopped.load(std::memory_order_acquire)) {
+ const int64_t deadline = _deadline.load(std::memory_order_acquire);
+ if (_claim_timeout(deadline, MonotonicMillis())) {
+ // Do not log, flush, invoke fatal-signal handlers, destroy
objects or
+ // join threads here: any of those can wait on the failed disk.
+ _exit(kTimeoutExitCode);
Review Comment:
[P2] The timeout exit is silent under the default launcher
On timeout the process leaves no log, no core and nothing in `be.out`. With
`bin/start_be.sh --daemon` (`nohup ... &`, lines 681/683) nothing collects the
exit status either, so the BE just disappears. This looks the same as an
external `SIGKILL`, and the operator cannot tell which disk hung. The header
comment "The process supervisor can report this exit status"
(`disk_health_check_watchdog.h:34-36`) only holds under systemd/k8s/foreground
runs.
A way to keep the never-block guarantee:
- Let `ScopedCheck` carry a stable label (e.g. the `DataDir` path's
`c_str()`).
- On timeout, hand a pre-formatted message to a helper thread created in
`start()`; the helper `write()`s it to stderr.
- The watchdog waits only a short bounded grace period (e.g. 200 ms), then
calls `_exit(74)` unconditionally, even if the helper's write blocks.
Setting `O_NONBLOCK` on a `dup(2)` would not work: it has no effect on
regular files such as `be.out`, and it belongs to the shared open file
description, so it would also make fd 2 non-blocking process-wide.
At minimum, please qualify the header comment and document exit status 74 in
the config reference.
##########
be/src/storage/disk_health_check_watchdog.cpp:
##########
@@ -0,0 +1,128 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "storage/disk_health_check_watchdog.h"
+
+#include <unistd.h>
+
+#include <ctime>
+#include <limits>
+
+#include "common/check.h"
+#include "common/status.h"
+#include "util/time.h"
+
+namespace doris {
+
+static_assert(std::atomic<int64_t>::is_always_lock_free);
+static_assert(std::atomic<bool>::is_always_lock_free);
+
+DiskHealthCheckWatchdog::~DiskHealthCheckWatchdog() {
+ stop();
+}
+
+Status DiskHealthCheckWatchdog::start(std::chrono::milliseconds timeout) {
+ // NOLINTNEXTLINE(readability-inconsistent-ifelse-braces): DORIS_CHECK
expands to if/else.
+ DORIS_CHECK(!_started && !_stopped.load(std::memory_order_acquire));
+ if (timeout.count() < 0 || timeout.count() >
std::numeric_limits<int32_t>::max() * 1000LL) {
+ return Status::InvalidArgument("Invalid disk health check timeout: {}
ms", timeout.count());
+ }
+ _timeout_ms = timeout.count();
+ if (_timeout_ms == 0) {
+ return Status::OK();
+ }
+ // Unlike ordinary Doris workers, this thread must work before ExecEnv is
+ // initialized and cannot depend on ThreadMgr locks or memory-tracker
setup.
+ // Its callback neither allocates nor logs and needs no Doris thread
context.
+ int err = pthread_create(
Review Comment:
[P3] Name the poller thread
The raw pthread inherits the process name, so in `top -H`,
`/proc/<pid>/task` and gdb it shows up as `doris_be` and is hard to identify. A
`pthread_setname_np` call at thread start has no blocking risk.
##########
be/src/storage/disk_health_check_watchdog.cpp:
##########
@@ -0,0 +1,128 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "storage/disk_health_check_watchdog.h"
+
+#include <unistd.h>
+
+#include <ctime>
+#include <limits>
+
+#include "common/check.h"
+#include "common/status.h"
+#include "util/time.h"
+
+namespace doris {
+
+static_assert(std::atomic<int64_t>::is_always_lock_free);
+static_assert(std::atomic<bool>::is_always_lock_free);
+
+DiskHealthCheckWatchdog::~DiskHealthCheckWatchdog() {
+ stop();
+}
+
+Status DiskHealthCheckWatchdog::start(std::chrono::milliseconds timeout) {
+ // NOLINTNEXTLINE(readability-inconsistent-ifelse-braces): DORIS_CHECK
expands to if/else.
+ DORIS_CHECK(!_started && !_stopped.load(std::memory_order_acquire));
+ if (timeout.count() < 0 || timeout.count() >
std::numeric_limits<int32_t>::max() * 1000LL) {
Review Comment:
[P3] Defensive range check is unreachable from production callers
Both callers pass
`std::chrono::seconds(config::disk_health_check_timeout_seconds)`. The
validator already guarantees `>= 0`, and an `int32` number of seconds cannot
exceed the upper bound. Per AGENTS.md (no defensive `if` without a known
failure path), consider a `DORIS_CHECK` here and dropping
`RejectsInvalidTimeouts`. The `pthread_create` error check below is fine to
keep, since `EAGAIN` can really happen.
--
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]
