Copilot commented on code in PR #65362:
URL: https://github.com/apache/doris/pull/65362#discussion_r3542120751
##########
be/src/load/group_commit/group_commit_mgr.cpp:
##########
@@ -307,7 +312,34 @@ Status GroupCommitTable::get_first_block_load_queue(
_create_plan_deps.emplace(load_id, std::make_tuple(create_plan_dep,
put_block_dep,
base_schema_version,
index_size));
[[maybe_unused]] auto submit_st = _submit_create_group_commit_load();
- return try_to_get_matched_queue();
+ RETURN_IF_ERROR(try_to_get_matched_queue(need_create_plan));
+ if (need_create_plan) {
+ return Status::InternalError<false>("can not get a block queue for
table_id: " +
+ std::to_string(_table_id) +
_create_plan_failed_reason);
+ }
+ return Status::OK();
+}
+
+Status GroupCommitTable::_check_wal_backlog(TGroupCommitMode::type
group_commit_mode) {
+ int32_t max_wal_num = config::group_commit_max_wal_num_per_table;
+ if (group_commit_mode != TGroupCommitMode::ASYNC_MODE || max_wal_num <= 0)
{
+ return Status::OK();
+ }
+ size_t wal_num = _exec_env->wal_mgr()->get_wal_queue_size(_table_id);
+ if (wal_num < static_cast<size_t>(max_wal_num)) {
Review Comment:
`_check_wal_backlog()` is on the load admission path for async group commit
and calls `WalManager::get_wal_queue_size()`. That function currently takes an
exclusive `_wal_queue_lock` (`std::lock_guard<std::shared_mutex>`), which can
serialize concurrent admissions and increase contention under load. Consider
switching the read path in `get_wal_queue_size()` to a `std::shared_lock`
(keeping exclusive locking for add/erase).
##########
be/src/common/config.cpp:
##########
@@ -1435,6 +1435,9 @@ DEFINE_mInt32(group_commit_queue_mem_limit, "67108864");
// group_commit_wal_max_disk_limit=1024 or group_commit_wal_max_disk_limit=10%
can be automatically identified.
DEFINE_String(group_commit_wal_max_disk_limit, "10%");
DEFINE_Bool(group_commit_wait_replay_wal_finish, "false");
+// Max WAL count for one table before rejecting async group commit loads.
+// 0 means no limit.
+DEFINE_mInt32(group_commit_max_wal_num_per_table, "10");
Review Comment:
This introduces a new backpressure mechanism, but the default value (`"10"`)
enables rejections immediately after upgrade even if operators haven't opted
in. If backward-compatibility is desired, consider defaulting this config to
`0` (disabled) and letting operators enable/tune it explicitly (or clearly
documenting the behavior change).
##########
be/src/load/group_commit/wal/wal_table.cpp:
##########
@@ -113,6 +113,13 @@ Status WalTable::_relay_wal_one_by_one() {
doris::wal_fail << 1;
LOG(WARNING) << "failed to replay wal=" << wal_info->get_wal_path()
<< ", st=" << st.to_string();
+ {
+ std::lock_guard<std::mutex> lock(_replay_wal_lock);
+ _last_replay_wal_failed_reason = "failed to replay wal=" +
+ wal_info->get_wal_path() +
+ ", st=" +
+ st.to_string().substr(0, 100);
+ }
need_retry_wals.push_back(wal_info);
Review Comment:
The new `_last_replay_wal_failed_reason` is protected by `_replay_wal_lock`
here, but `_relay_wal_one_by_one()` still iterates `_replaying_queue` without
holding `_replay_wal_lock`. Since `WalManager::_replay_background()` submits
`replay_wals()` tasks via a thread pool periodically, two `replay_wals()` calls
for the same table can overlap and concurrently iterate/mutate
`_replaying_queue`, causing a data race (and potentially inconsistent/incorrect
failure reasons).
--
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]
