github-actions[bot] commented on code in PR #66250:
URL: https://github.com/apache/doris/pull/66250#discussion_r3680158771
##########
be/src/runtime/runtime_state.cpp:
##########
@@ -394,13 +398,18 @@ Status
RuntimeState::append_error_msg_to_file(std::function<std::string()> line,
return Status::OK();
}
+std::string RuntimeState::get_first_error_msg() const {
+ std::lock_guard<std::mutex> l(_load_error_log_lock);
+ return _first_error_msg;
+}
+
std::string RuntimeState::get_error_log_file_path() {
+ std::lock_guard<std::mutex> l(_load_error_log_lock);
Review Comment:
Please avoid holding `_load_error_log_lock` across the synchronous S3 upload
and presigning. `send_report(false)` runs this path on a pipeline scheduler
pthread, where `RemoteFileSystem::upload()` executes inline, while the
FragmentMgr async writer now blocks at `append_error_msg_to_file()`. With up to
roughly 51 rows and a default 200 MiB per-row log limit, a report can stall
both the scheduler and the writer for a multi-gigabyte remote transfer. Under
the mutex, flush/rotate or snapshot immutable state; release it for remote I/O,
then publish the result with a state check.
##########
be/src/runtime/runtime_state.cpp:
##########
@@ -349,6 +349,7 @@ Status
RuntimeState::append_error_msg_to_file(std::function<std::string()> line,
if (query_type() != TQueryType::LOAD) {
return Status::OK();
}
+ std::lock_guard<std::mutex> l(_load_error_log_lock);
Review Comment:
Please keep the active writer valid after a periodic upload. This mutex only
serializes the bad sequence: an async writer appends one invalid row; periodic
`get_error_log_file_path()` acquires the lock, closes/uploads the stream, and
replaces the local path with the presigned URL; then the writer reacquires this
lock for another row, sees `_error_log_file != nullptr`, and writes to the
closed `ofstream`. No stream state is checked, so the append returns `OK` while
the published object never receives later rows. Please use an explicit
active/finalized or rotating-snapshot state (and keep the local path separate
from the URL), or defer the final close until writers have finished.
##########
regression-test/suites/fault_injection_p0/cloud/test_load_error_log_concurrent_write.groovy:
##########
@@ -0,0 +1,84 @@
+// 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.
+
+suite("test_load_error_log_concurrent_write", "nonConcurrent") {
+ if (!isCloudMode()) {
+ return
+ }
+
+ sql "DROP TABLE IF EXISTS test_load_error_log_concurrent_write_source"
+ sql "DROP TABLE IF EXISTS test_load_error_log_concurrent_write_target"
+ sql """
+ CREATE TABLE test_load_error_log_concurrent_write_source (
+ id INT,
+ value STRING
+ )
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES ("replication_num" = "1")
+ """
+ sql """
+ CREATE TABLE test_load_error_log_concurrent_write_target (
+ id INT,
+ value VARCHAR(4)
+ )
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES ("replication_num" = "1")
+ """
+ sql """
+ INSERT INTO test_load_error_log_concurrent_write_source
+ VALUES (1, 'value-too-long')
+ """
+ sql "SET enable_insert_strict = true"
+ sql "SET enable_insert_value_auto_cast = false"
+
+ setBeConfigTemporary([
+ save_load_error_log_to_s3: true,
+ pipeline_status_report_interval: 1
+ ]) {
+ try {
+ GetDebugPoint().clearDebugPointsForAllBEs()
+ // Keep the first writer between opening the file and writing its
first row long enough
+ // for periodic status reporting to request the error log URL
concurrently.
+ GetDebugPoint().enableDebugPointForAllBEs(
Review Comment:
Please make this race deterministic. There is no handshake proving that the
writer reached this sleep before the reporter inspected the state. If reporting
runs first while `_error_log_file` is still null, it returns no URL; the writer
later writes normally and the final report uploads a complete log, so every
assertion passes without exercising the old race. The new sleep point is also
absent on the base revision, so the exact test can pass there. Use a two-party
barrier or hit assertions (ideally a BE unit test with latches, or coordinated
debug-only signals) that proves both sides overlap and fails when only the
synchronization fix is reverted.
--
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]
