github-actions[bot] commented on code in PR #28918: URL: https://github.com/apache/doris/pull/28918#discussion_r1435516102
########## be/src/agent/workload_move_action_listener.h: ########## @@ -0,0 +1,40 @@ +// 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. + +#pragma once + +#include <glog/logging.h> + +#include "agent/topic_listener.h" +#include "pipeline/pipeline_fragment_context.h" +#include "pipeline/task_queue.h" +#include "runtime/exec_env.h" +#include "runtime/fragment_mgr.h" +#include "runtime/task_group/task_group_manager.h" + +namespace doris { +class WorkloadMoveActionListener : public TopicListener { +public: + ~WorkloadMoveActionListener() {} Review Comment: warning: use '= default' to define a trivial destructor [modernize-use-equals-default] ```suggestion ~WorkloadMoveActionListener() = default; ``` ########## be/src/runtime/task_group/task_group_manager.cpp: ########## @@ -237,6 +238,21 @@ LOG(INFO) << "finish clear unused cgroup path"; } +bool TaskGroupManager::migrate_memory_tracker_to_group( + std::shared_ptr<MemTrackerLimiter> mem_tracker, uint64_t src_group_id, + uint64_t dst_group_id, TaskGroupPtr* dst_group_ptr) { + std::lock_guard<std::shared_mutex> write_lock(_group_mutex); + if (_task_groups.find(src_group_id) == _task_groups.end() || + _task_groups.find(dst_group_id) == _task_groups.end()) { + return false; + } + _task_groups[src_group_id]->remove_mem_tracker_limiter(mem_tracker); + *dst_group_ptr = _task_groups[dst_group_id]; + (*dst_group_ptr)->add_mem_tracker_limiter(mem_tracker); + + return true; +} + void TaskGroupManager::stop() { Review Comment: warning: method 'reset_query_ctx_scan_scheduler' can be made static [readability-convert-member-functions-to-static] be/src/runtime/task_group/task_group_manager.h:85: ```diff - void reset_query_ctx_scan_scheduler(QueryContext* query_ctx, uint64_t dst_group_id); + static void reset_query_ctx_scan_scheduler(QueryContext* query_ctx, uint64_t dst_group_id); ``` ########## be/src/agent/workload_move_action_listener.h: ########## @@ -0,0 +1,40 @@ +// 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. + +#pragma once + +#include <glog/logging.h> Review Comment: warning: 'glog/logging.h' file not found [clang-diagnostic-error] ```cpp #include <glog/logging.h> ^ ``` ########## be/src/runtime/fragment_mgr.h: ########## @@ -153,6 +153,15 @@ class FragmentMgr : public RestMonitorIface { std::string dump_pipeline_tasks(); + void get_query_ctx_by_query_id(TUniqueId query_id, Review Comment: warning: method 'get_query_ctx_by_query_id' can be made static [readability-convert-member-functions-to-static] ```suggestion static void get_query_ctx_by_query_id(TUniqueId query_id, ``` ########## be/src/pipeline/task_scheduler.h: ########## @@ -93,6 +93,10 @@ class TaskScheduler { TaskQueue* task_queue() const { return _task_queue.get(); } + void set_wg_id(uint64_t wg_id) { this->_wg_id = wg_id; } + + uint64_t get_wg_id() { return _wg_id; } Review Comment: warning: method 'get_wg_id' can be made const [readability-make-member-function-const] ```suggestion uint64_t get_wg_id() const { return _wg_id; } ``` ########## be/src/runtime/task_group/task_group_manager.cpp: ########## @@ -237,6 +238,21 @@ void TaskGroupManager::delete_task_group_by_ids(std::set<uint64_t> used_wg_id) { LOG(INFO) << "finish clear unused cgroup path"; } +bool TaskGroupManager::migrate_memory_tracker_to_group( Review Comment: warning: method 'migrate_memory_tracker_to_group' can be made static [readability-convert-member-functions-to-static] be/src/runtime/task_group/task_group_manager.h:81: ```diff - bool migrate_memory_tracker_to_group(std::shared_ptr<MemTrackerLimiter> mem_tracker, + static bool migrate_memory_tracker_to_group(std::shared_ptr<MemTrackerLimiter> mem_tracker, ``` ########## be/src/runtime/query_context.h: ########## @@ -149,9 +150,20 @@ class QueryContext { vectorized::RuntimePredicate& get_runtime_predicate() { return _runtime_predicate; } - void set_task_group(taskgroup::TaskGroupPtr& tg) { _task_group = tg; } + void set_task_group(taskgroup::TaskGroupPtr& tg) { + std::lock_guard<std::shared_mutex> write_lock(_task_group_lock); + _task_group = tg; + } + + taskgroup::TaskGroup* get_task_group() { Review Comment: warning: method 'get_task_group' can be made static [readability-convert-member-functions-to-static] ```suggestion static taskgroup::TaskGroup* get_task_group() { ``` ########## be/src/runtime/query_context.h: ########## @@ -192,17 +204,31 @@ TUniqueId query_id() const { return _query_id; } - void set_task_scheduler(pipeline::TaskScheduler* task_scheduler) { - _task_scheduler = task_scheduler; + void set_task_scheduler(std::shared_ptr<pipeline::TaskScheduler>* task_scheduler) { + std::lock_guard<std::shared_mutex> write_lock(_exec_task_sched_mutex); + _task_scheduler = *task_scheduler; } - pipeline::TaskScheduler* get_task_scheduler() { return _task_scheduler; } + pipeline::TaskScheduler* get_task_scheduler() { Review Comment: warning: method 'get_task_scheduler' can be made static [readability-convert-member-functions-to-static] ```suggestion static pipeline::TaskScheduler* get_task_scheduler() { ``` ########## be/src/runtime/task_group/task_group_manager.cpp: ########## @@ -237,6 +238,21 @@ LOG(INFO) << "finish clear unused cgroup path"; } +bool TaskGroupManager::migrate_memory_tracker_to_group( + std::shared_ptr<MemTrackerLimiter> mem_tracker, uint64_t src_group_id, + uint64_t dst_group_id, TaskGroupPtr* dst_group_ptr) { + std::lock_guard<std::shared_mutex> write_lock(_group_mutex); + if (_task_groups.find(src_group_id) == _task_groups.end() || + _task_groups.find(dst_group_id) == _task_groups.end()) { + return false; Review Comment: warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr] be/src/runtime/task_group/task_group_manager.cpp:244: ```diff - if (_task_groups.find(src_group_id) == _task_groups.end() || - _task_groups.find(dst_group_id) == _task_groups.end()) { - return false; - } - _task_groups[src_group_id]->remove_mem_tracker_limiter(mem_tracker); - *dst_group_ptr = _task_groups[dst_group_id]; - (*dst_group_ptr)->add_mem_tracker_limiter(mem_tracker); - - return true; + return !_task_groups.find(src_group_id) == _task_groups.end() || + _task_groups.find(dst_group_id) == _task_groups.end(); ``` ########## be/src/runtime/task_group/task_group_manager.h: ########## @@ -71,15 +71,27 @@ class TaskGroupManager { // doris task group only support cpu soft limit bool enable_cgroup() { return enable_cpu_hard_limit() || config::enable_cgroup_cpu_soft_limit; } + pipeline::TaskQueue* get_task_queue_by_id(uint64_t group_id) { Review Comment: warning: method 'get_task_queue_by_id' can be made static [readability-convert-member-functions-to-static] ```suggestion static pipeline::TaskQueue* get_task_queue_by_id(uint64_t group_id) { ``` ########## be/src/runtime/query_context.h: ########## @@ -192,17 +204,31 @@ TUniqueId query_id() const { return _query_id; } - void set_task_scheduler(pipeline::TaskScheduler* task_scheduler) { - _task_scheduler = task_scheduler; + void set_task_scheduler(std::shared_ptr<pipeline::TaskScheduler>* task_scheduler) { + std::lock_guard<std::shared_mutex> write_lock(_exec_task_sched_mutex); + _task_scheduler = *task_scheduler; } - pipeline::TaskScheduler* get_task_scheduler() { return _task_scheduler; } + pipeline::TaskScheduler* get_task_scheduler() { + std::shared_lock<std::shared_mutex> read_lock(_exec_task_sched_mutex); + if (_task_scheduler) { + return _task_scheduler.get(); + } + return nullptr; + } - void set_scan_task_scheduler(vectorized::SimplifiedScanScheduler* scan_task_scheduler) { - _scan_task_scheduler = scan_task_scheduler; + pipeline::TaskQueue* get_exec_task_queue(); + + void set_scan_task_scheduler( + std::shared_ptr<vectorized::SimplifiedScanScheduler>* scan_task_scheduler) { + std::lock_guard<std::shared_mutex> write_lock(_scan_task_sched_mutex); + _scan_task_scheduler = *scan_task_scheduler; } - vectorized::SimplifiedScanScheduler* get_scan_scheduler() { return _scan_task_scheduler; } + vectorized::SimplifiedScanScheduler* get_scan_scheduler() { Review Comment: warning: method 'get_scan_scheduler' can be made static [readability-convert-member-functions-to-static] ```suggestion static vectorized::SimplifiedScanScheduler* get_scan_scheduler() { ``` ########## be/src/runtime/query_context.h: ########## @@ -149,9 +150,20 @@ vectorized::RuntimePredicate& get_runtime_predicate() { return _runtime_predicate; } - void set_task_group(taskgroup::TaskGroupPtr& tg) { _task_group = tg; } + void set_task_group(taskgroup::TaskGroupPtr& tg) { + std::lock_guard<std::shared_mutex> write_lock(_task_group_lock); + _task_group = tg; + } + + taskgroup::TaskGroup* get_task_group() { + std::shared_lock<std::shared_mutex> read_lock(_task_group_lock); + return _task_group.get(); + } - taskgroup::TaskGroup* get_task_group() const { return _task_group.get(); } + taskgroup::TaskGroupPtr get_task_group_shared_ptr() { Review Comment: warning: method 'get_task_group_shared_ptr' can be made static [readability-convert-member-functions-to-static] ```suggestion static taskgroup::TaskGroupPtr get_task_group_shared_ptr() { ``` -- 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]
