github-actions[bot] commented on code in PR #68385:
URL: https://github.com/apache/doris/pull/68385#discussion_r4091517435
##########
be/src/storage/tablet/base_tablet.cpp:
##########
@@ -1745,15 +1746,15 @@ Status BaseTablet::update_delete_bitmap(const
BaseTabletSPtr& self, TabletTxnInf
transient_rs_writer = std::move(group_writer);
}
- // When there is only one segment, it will be calculated in the current
thread.
- // Otherwise, it will be submitted to the thread pool for calculation.
- if (segments.size() <= 1) {
+ // Preserve the local single-segment fast path while holding the tablet
lock.
+ // Load workers submit leaves and help them without another
resource-context attach.
+ if (segments.size() <= 1 && ThreadPool::current_load_pool() == nullptr) {
RETURN_IF_ERROR(calc_delete_bitmap(self, rowset, segments,
specified_rowsets, delete_bitmap,
cur_version - 1, nullptr,
transient_rs_writer.get(),
tablet_delete_bitmap));
-
} else {
- auto token = self->calc_delete_bitmap_executor()->create_token();
+ auto token = self->calc_delete_bitmap_executor()->create_load_token(
+ txn_id, LoadTaskPriority::HIGHEST, LoadTaskType::LEAF);
Review Comment:
[P2] Preserve the originating workload group across local commit/publish and
pass it to the explicit `create_load_token(..., wg)` overload here (and at the
earlier skip-alignment call). Both local publish tasks attach only a fresh mem
tracker, while `TabletTxnInfo` retains no workload group, so this implicit
overload observes a null group and routes multi-segment MoW publish work to the
global `_load_pool`. That bypasses workload-group isolation for the newly
consolidated foreground work. Please carry the owner in local transaction state
with the same dropped-group fallback used by cloud publish, and cover normal
plus async local publish.
##########
be/src/common/config.cpp:
##########
@@ -857,14 +857,14 @@ DEFINE_mInt32(storage_flood_stage_usage_percent, "90");
// 90%
DEFINE_mInt64(storage_flood_stage_left_capacity_bytes, "1073741824"); // 1GB
// number of thread for flushing memtable per store
DEFINE_mInt32(flush_thread_num_per_store, "6");
-// number of thread for flushing memtable per store, for high priority load
task
+// Deprecated compatibility setting; foreground load tasks share the normal
flush pool.
DEFINE_mInt32(high_priority_flush_thread_num_per_store, "6");
-// number of threads = min(flush_thread_num_per_store * num_store,
-// max_flush_thread_num_per_cpu * num_cpu)
-DEFINE_mInt32(max_flush_thread_num_per_cpu, "4");
+// Maximum shared foreground load threads per CPU (default 8).
+// Without adaptive flushing, also capped by flush_thread_num_per_store *
num_store.
+DEFINE_mInt32(max_flush_thread_num_per_cpu, "8");
Review Comment:
[P2] This default is also reused by `ExecEnv` to cap `LoadStreamMgr`'s
`SegmentFileWriterThreadPool`, which sets the result as both its minimum and
maximum; `ThreadPool::init()` eagerly creates that minimum. Raising 4 to 8
therefore enlarges a separate fixed startup pool (for example, 8 CPUs and 8
stores goes from 32 to 48 threads), even though this key is now documented as
the shared foreground-load cap and the new tests cover only that pool. Please
decouple segment-writer sizing or preserve its old effective cap with a
dedicated setting, and document/test the intended startup behavior.
--
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]
