wenzhenghu commented on code in PR #68740:
URL: https://github.com/apache/doris/pull/68740#discussion_r4267284487
##########
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:
Documented in
[04d211fa61bd](https://github.com/apache/doris/commit/04d211fa61bdd16bc288a448379c0de6f97021d4)
and the PR release/operator guidance. One protected operation timing out
terminates the whole BE independently of the error-disk percentage. A blocked
probe cannot finish broken-path persistence, so restarting against the same
storage can repeat the timeout.
Recovery guidance is to repair the storage or exclude the affected path from
both data and spill configuration before restarting. Completed IO-error
handling and the disabled default remain unchanged.
##########
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:
I kept the range validation and `RejectsInvalidTimeouts` deliberately. The
current configuration callers supply validated int32 seconds, as you noted, but
the public `start(std::chrono::milliseconds)` interface accepts millisecond
durations, and its tests exercise invalid negative/out-of-range inputs. Its
contract is to return `InvalidArgument` before creating a thread for those
inputs.
This is startup input validation, outside the polling loop. Changing the
interface to an assertion-only contract was not adopted in this update; the
real `pthread_create` failure handling also remains.
--
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]
