You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "Gabriel39 (via GitHub)" <gi...@apache.org> on 2024/03/17 12:12:48 UTC
[PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Gabriel39 opened a new pull request, #32346:
URL: https://github.com/apache/doris/pull/32346
## Proposed changes
Issue Number: close #xxx
<!--Describe your changes.-->
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002751617
clang-tidy review says "All clean, LGTM! :+1:"
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1527778789
##########
be/src/runtime/fragment_mgr.cpp:
##########
@@ -577,12 +578,18 @@ void FragmentMgr::remove_pipeline_context(
_query_ctx_map.erase(query_id);
}
}
- {
- std::lock_guard<std::mutex> plock(q_context->pipeline_lock);
- if (q_context->fragment_id_to_pipeline_ctx.contains(f_context->get_fragment_id())) {
- q_context->fragment_id_to_pipeline_ctx.erase(f_context->get_fragment_id());
- }
+}
+
+Status FragmentMgr::get_query_ctx(const TPipelineFragmentParams& params, TUniqueId query_id,
Review Comment:
warning: method 'get_query_ctx' can be made static [readability-convert-member-functions-to-static]
```suggestion
static Status FragmentMgr::get_query_ctx(const TPipelineFragmentParams& params, TUniqueId query_id,
```
##########
be/src/runtime/fragment_mgr.cpp:
##########
@@ -773,6 +779,7 @@ std::string FragmentMgr::dump_pipeline_tasks() {
}
Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
Review Comment:
warning: function 'exec_plan_fragment' has cognitive complexity of 88 (threshold 50) [readability-function-cognitive-complexity]
```cpp
Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
^
```
<details>
<summary>Additional context</summary>
**be/src/runtime/fragment_mgr.cpp:790:** +1
```cpp
const bool enable_pipeline_x = params.query_options.__isset.enable_pipeline_x_engine &&
^
```
**be/src/runtime/fragment_mgr.cpp:792:** +1, including nesting penalty of 0, nesting level increased to 1
```cpp
if (enable_pipeline_x) {
^
```
**be/src/runtime/fragment_mgr.cpp:804:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (!prepare_st.ok()) {
^
```
**be/src/runtime/fragment_mgr.cpp:812:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
for (size_t i = 0; i < params.local_params.size(); i++) {
^
```
**be/src/runtime/fragment_mgr.cpp:814:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:814:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:817:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!i && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:817:** +1
```cpp
if (!i && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:824:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (iter != _pipeline_map.end()) {
^
```
**be/src/runtime/fragment_mgr.cpp:831:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:831:** +1
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:846:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (!query_ctx->fragment_id_to_pipeline_ctx.contains(params.fragment_id)) {
^
```
**be/src/runtime/fragment_mgr.cpp:853:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
RETURN_IF_ERROR(context->submit());
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:853:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
RETURN_IF_ERROR(context->submit());
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:855:** +1, nesting level increased to 1
```cpp
} else {
^
```
**be/src/runtime/fragment_mgr.cpp:856:** nesting level increased to 2
```cpp
auto pre_and_submit = [&](int i) {
^
```
**be/src/runtime/fragment_mgr.cpp:863:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (iter != _pipeline_map.end()) {
^
```
**be/src/runtime/fragment_mgr.cpp:871:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:871:** +1
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:886:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!prepare_st.ok()) {
^
```
**be/src/runtime/fragment_mgr.cpp:896:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:896:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:899:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (i == 0 && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:899:** +1
```cpp
if (i == 0 && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:914:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (local_params.__isset.runtime_filter_params) {
^
```
**be/src/runtime/fragment_mgr.cpp:915:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (local_params.__isset.runtime_filter_params) {
^
```
**be/src/runtime/fragment_mgr.cpp:920:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (local_params.__isset.topn_filter_source_node_ids) {
^
```
**be/src/runtime/fragment_mgr.cpp:922:** +1, nesting level increased to 2
```cpp
} else {
^
```
**be/src/runtime/fragment_mgr.cpp:926:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (target_size > 1) {
^
```
**be/src/runtime/fragment_mgr.cpp:932:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
for (size_t i = 0; i < target_size; i++) {
^
```
**be/src/runtime/fragment_mgr.cpp:933:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:933:** +5, including nesting penalty of 4, nesting level increased to 5
```cpp
RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:944:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (prepare_done != target_size) {
^
```
**be/src/runtime/fragment_mgr.cpp:947:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
for (size_t i = 0; i < target_size; i++) {
^
```
**be/src/runtime/fragment_mgr.cpp:948:** +5, including nesting penalty of 4, nesting level increased to 5
```cpp
if (!prepare_status[i].ok()) {
^
```
**be/src/runtime/fragment_mgr.cpp:954:** +1, nesting level increased to 2
```cpp
} else {
^
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "Gabriel39 (via GitHub)" <gi...@apache.org>.
Gabriel39 commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002885934
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1528028084
##########
be/src/runtime/query_context.h:
##########
@@ -61,6 +61,11 @@ struct ReportStatusRequest {
std::function<void(const PPlanFragmentCancelReason&, const std::string&)> cancel_fn;
};
+struct PipelineFragmentContextWrapper {
+ bool valid = false;
Review Comment:
if valid variable is useless, then the wrapper is useless
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1527691537
##########
be/src/runtime/fragment_mgr.cpp:
##########
@@ -1044,12 +1033,18 @@ void FragmentMgr::cancel_fragment_unlocked(const TUniqueId& query_id, int32_t fr
const PPlanFragmentCancelReason& reason,
const std::unique_lock<std::mutex>& state_lock,
const std::string& msg) {
- auto q_ctx = _query_ctx_map.find(query_id)->second;
- auto f_context = q_ctx->fragment_id_to_pipeline_ctx.find(fragment_id);
- if (f_context != q_ctx->fragment_id_to_pipeline_ctx.end()) {
- f_context->second->cancel(reason, msg);
+ if (auto q_ctx = _query_ctx_map.find(query_id)->second) {
+ auto f_context = q_ctx->fragment_id_to_pipeline_ctx.find(fragment_id);
+ if (f_context != q_ctx->fragment_id_to_pipeline_ctx.end()) {
Review Comment:
Add a new method in query context , not use the structure fragment_id_to_pipeline_ctx directly. We could not ensure the concurrency logic for the structure.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "Gabriel39 (via GitHub)" <gi...@apache.org>.
Gabriel39 commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002747879
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "Gabriel39 (via GitHub)" <gi...@apache.org>.
Gabriel39 commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002889576
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "Gabriel39 (via GitHub)" <gi...@apache.org>.
Gabriel39 commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002434701
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "Gabriel39 (via GitHub)" <gi...@apache.org>.
Gabriel39 commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002906042
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1528027286
##########
be/src/pipeline/pipeline_fragment_context.h:
##########
@@ -51,14 +51,17 @@ namespace pipeline {
class PipelineFragmentContext : public TaskExecutionContext {
public:
+ ENABLE_FACTORY_CREATOR(PipelineFragmentContext);
// Callback to report execution status of plan fragment.
// 'profile' is the cumulative profile, 'done' indicates whether the execution
// is done or still continuing.
// Note: this does not take a const RuntimeProfile&, because it might need to call
// functions like PrettyPrint() or to_thrift(), neither of which is const
// because they take locks.
+ static std::shared_ptr<PipelineFragmentContext> fake_context;
Review Comment:
Do not use global static variable, we do not know when it is deconstruted.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1528001157
##########
be/src/runtime/query_context.h:
##########
@@ -107,6 +112,11 @@ class QueryContext {
void set_ready_to_execute(bool is_cancelled);
[[nodiscard]] bool is_cancelled() const { return _is_cancelled.load(); }
+
+ using ForEachFunc = std::function<void(const int, PipelineFragmentContextWrapper&)>;
+ void for_each_pipeline_context(ForEachFunc for_each_func);
+ using MaphFunc = std::function<void(PipelineFragmentContextWrapper&)>;
+ Status map_pipeline_context(MaphFunc map_func, const int fragment_id);
Review Comment:
warning: parameter 'fragment_id' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]
```suggestion
Status map_pipeline_context(MaphFunc map_func, int fragment_id);
```
##########
be/src/runtime/query_context.cpp:
##########
@@ -161,18 +165,44 @@ bool QueryContext::cancel(bool v, std::string msg, Status new_status, int fragme
_is_cancelled.store(v);
set_ready_to_execute(true);
- {
- std::lock_guard<std::mutex> plock(pipeline_lock);
- for (auto& ctx : fragment_id_to_pipeline_ctx) {
- if (fragment_id == ctx.first) {
- continue;
- }
- ctx.second->cancel(PPlanFragmentCancelReason::INTERNAL_ERROR, msg);
+ for_each_pipeline_context([&](const int f_id, PipelineFragmentContextWrapper& ctx) -> void {
+ if (fragment_id == f_id) {
+ return;
}
- }
+ if (!ctx.valid) {
+ return;
+ }
+ if (auto pipeline_ctx = ctx.pip_ctx.lock()) {
+ pipeline_ctx->cancel(PPlanFragmentCancelReason::INTERNAL_ERROR, msg);
+ }
+ });
return true;
}
+void QueryContext::for_each_pipeline_context(ForEachFunc for_each_func) {
+ std::lock_guard<std::mutex> lock(_pipeline_map_write_lock);
+ for (auto& [f_id, f_context] : _fragment_id_to_pipeline_ctx) {
+ for_each_func(f_id, f_context);
+ }
+}
+
+Status QueryContext::map_pipeline_context(MaphFunc map_func, const int fragment_id) {
Review Comment:
warning: method 'map_pipeline_context' can be made static [readability-convert-member-functions-to-static]
be/src/runtime/query_context.h:118:
```diff
- Status map_pipeline_context(MaphFunc map_func, const int fragment_id);
+ static Status map_pipeline_context(MaphFunc map_func, const int fragment_id);
```
##########
be/src/runtime/fragment_mgr.cpp:
##########
@@ -773,6 +775,7 @@ std::string FragmentMgr::dump_pipeline_tasks() {
}
Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
Review Comment:
warning: function 'exec_plan_fragment' has cognitive complexity of 91 (threshold 50) [readability-function-cognitive-complexity]
```cpp
Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
^
```
<details>
<summary>Additional context</summary>
**be/src/runtime/fragment_mgr.cpp:786:** +1
```cpp
const bool enable_pipeline_x = params.query_options.__isset.enable_pipeline_x_engine &&
^
```
**be/src/runtime/fragment_mgr.cpp:788:** +1, including nesting penalty of 0, nesting level increased to 1
```cpp
if (enable_pipeline_x) {
^
```
**be/src/runtime/fragment_mgr.cpp:800:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (!prepare_st.ok()) {
^
```
**be/src/runtime/fragment_mgr.cpp:808:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
for (size_t i = 0; i < params.local_params.size(); i++) {
^
```
**be/src/runtime/fragment_mgr.cpp:810:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:810:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:813:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!i && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:813:** +1
```cpp
if (!i && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:820:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (iter != _pipeline_map.end()) {
^
```
**be/src/runtime/fragment_mgr.cpp:827:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:827:** +1
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:842:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
RETURN_IF_ERROR(query_ctx->map_pipeline_context(
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:842:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
RETURN_IF_ERROR(query_ctx->map_pipeline_context(
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:849:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
RETURN_IF_ERROR(context->submit());
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:849:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
RETURN_IF_ERROR(context->submit());
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:851:** +1, nesting level increased to 1
```cpp
} else {
^
```
**be/src/runtime/fragment_mgr.cpp:852:** nesting level increased to 2
```cpp
auto pre_and_submit = [&](int i) {
^
```
**be/src/runtime/fragment_mgr.cpp:859:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (iter != _pipeline_map.end()) {
^
```
**be/src/runtime/fragment_mgr.cpp:867:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:867:** +1
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:882:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!prepare_st.ok()) {
^
```
**be/src/runtime/fragment_mgr.cpp:892:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:892:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:895:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (i == 0 && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:895:** +1
```cpp
if (i == 0 && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:910:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (local_params.__isset.runtime_filter_params) {
^
```
**be/src/runtime/fragment_mgr.cpp:911:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (local_params.__isset.runtime_filter_params) {
^
```
**be/src/runtime/fragment_mgr.cpp:916:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (local_params.__isset.topn_filter_source_node_ids) {
^
```
**be/src/runtime/fragment_mgr.cpp:918:** +1, nesting level increased to 2
```cpp
} else {
^
```
**be/src/runtime/fragment_mgr.cpp:922:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (target_size > 1) {
^
```
**be/src/runtime/fragment_mgr.cpp:928:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
for (size_t i = 0; i < target_size; i++) {
^
```
**be/src/runtime/fragment_mgr.cpp:929:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:929:** +5, including nesting penalty of 4, nesting level increased to 5
```cpp
RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:940:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (prepare_done != target_size) {
^
```
**be/src/runtime/fragment_mgr.cpp:943:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
for (size_t i = 0; i < target_size; i++) {
^
```
**be/src/runtime/fragment_mgr.cpp:944:** +5, including nesting penalty of 4, nesting level increased to 5
```cpp
if (!prepare_status[i].ok()) {
^
```
**be/src/runtime/fragment_mgr.cpp:950:** +1, nesting level increased to 2
```cpp
} else {
^
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "Gabriel39 (via GitHub)" <gi...@apache.org>.
Gabriel39 commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2003249157
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "Gabriel39 (via GitHub)" <gi...@apache.org>.
Gabriel39 commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002747209
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002455712
TeamCity be ut coverage result:
Function Coverage: 34.95% (8576/24539)
Line Coverage: 26.67% (69548/260801)
Region Coverage: 25.95% (36118/139198)
Branch Coverage: 22.90% (18449/80576)
Coverage Report: http://coverage.selectdb-in.cc/coverage/70581e0a54a974d39e500005f96f9cba48b23d20_70581e0a54a974d39e500005f96f9cba48b23d20/report/index.html
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1527520091
##########
be/src/runtime/query_context.cpp:
##########
@@ -162,12 +162,11 @@ bool QueryContext::cancel(bool v, std::string msg, Status new_status, int fragme
set_ready_to_execute(true);
{
- std::lock_guard<std::mutex> plock(pipeline_lock);
for (auto& ctx : fragment_id_to_pipeline_ctx) {
if (fragment_id == ctx.first) {
continue;
}
- ctx.second->cancel(PPlanFragmentCancelReason::INTERNAL_ERROR, msg);
+ ctx.second.lock()->cancel(PPlanFragmentCancelReason::INTERNAL_ERROR, msg);
Review Comment:
lock may fail， should check lock result
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002438307
clang-tidy review says "All clean, LGTM! :+1:"
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1528104058
##########
be/src/runtime/query_context.h:
##########
@@ -107,6 +107,13 @@ class QueryContext {
void set_ready_to_execute(bool is_cancelled);
[[nodiscard]] bool is_cancelled() const { return _is_cancelled.load(); }
+
+ void cancel_all_pipeline_context(const PPlanFragmentCancelReason& reason,
+ const std::string& msg);
+ Status cancel_pipeline_context(const int fragment_id, const PPlanFragmentCancelReason& reason,
Review Comment:
warning: parameter 'fragment_id' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]
```suggestion
Status cancel_pipeline_context(int fragment_id, const PPlanFragmentCancelReason& reason,
```
##########
be/src/runtime/query_context.cpp:
##########
@@ -162,17 +166,48 @@ bool QueryContext::cancel(bool v, std::string msg, Status new_status, int fragme
set_ready_to_execute(true);
{
- std::lock_guard<std::mutex> plock(pipeline_lock);
- for (auto& ctx : fragment_id_to_pipeline_ctx) {
- if (fragment_id == ctx.first) {
+ std::lock_guard<std::mutex> lock(_pipeline_map_write_lock);
+ for (auto& [f_id, f_context] : _fragment_id_to_pipeline_ctx) {
+ if (fragment_id == f_id) {
continue;
}
- ctx.second->cancel(PPlanFragmentCancelReason::INTERNAL_ERROR, msg);
+ if (auto pipeline_ctx = f_context.lock()) {
+ pipeline_ctx->cancel(PPlanFragmentCancelReason::INTERNAL_ERROR, msg);
+ }
}
}
return true;
}
+void QueryContext::cancel_all_pipeline_context(const PPlanFragmentCancelReason& reason,
+ const std::string& msg) {
+ std::lock_guard<std::mutex> lock(_pipeline_map_write_lock);
+ for (auto& [f_id, f_context] : _fragment_id_to_pipeline_ctx) {
+ if (auto pipeline_ctx = f_context.lock()) {
+ pipeline_ctx->cancel(reason, msg);
+ }
+ }
+}
+
+Status QueryContext::cancel_pipeline_context(const int fragment_id,
Review Comment:
warning: method 'cancel_pipeline_context' can be made static [readability-convert-member-functions-to-static]
```suggestion
static Status QueryContext::cancel_pipeline_context(const int fragment_id,
```
##########
be/src/runtime/query_context.h:
##########
@@ -107,6 +107,13 @@
void set_ready_to_execute(bool is_cancelled);
[[nodiscard]] bool is_cancelled() const { return _is_cancelled.load(); }
+
+ void cancel_all_pipeline_context(const PPlanFragmentCancelReason& reason,
+ const std::string& msg);
+ Status cancel_pipeline_context(const int fragment_id, const PPlanFragmentCancelReason& reason,
+ const std::string& msg);
+ void set_pipeline_context(const int fragment_id,
Review Comment:
warning: parameter 'fragment_id' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]
```suggestion
void set_pipeline_context(int fragment_id,
```
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2003271075
PR approved by anyone and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei merged PR #32346:
URL: https://github.com/apache/doris/pull/32346
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002742929
clang-tidy review says "All clean, LGTM! :+1:"
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002434680
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2003330860
TeamCity be ut coverage result:
Function Coverage: 35.25% (8701/24687)
Line Coverage: 27.08% (71181/262903)
Region Coverage: 26.33% (36926/140264)
Branch Coverage: 23.25% (18890/81264)
Coverage Report: http://coverage.selectdb-in.cc/coverage/15d851743f16b9709892e5ffff6f91233d6a2ec3_15d851743f16b9709892e5ffff6f91233d6a2ec3/report/index.html
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2003270992
PR approved by at least one committer and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1527664509
##########
be/src/runtime/fragment_mgr.cpp:
##########
@@ -839,11 +833,8 @@ Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
_pipeline_map.insert({ins_id, context});
}
- _cv.notify_all();
- }
- {
- std::lock_guard<std::mutex> lock(query_ctx->pipeline_lock);
query_ctx->fragment_id_to_pipeline_ctx.insert({params.fragment_id, context});
+ _cv.notify_all();
Review Comment:
I do not find any call to _cv.wait， why do we need notify?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1527664948
##########
be/src/runtime/fragment_mgr.cpp:
##########
@@ -1047,7 +1038,7 @@ void FragmentMgr::cancel_fragment_unlocked(const TUniqueId& query_id, int32_t fr
auto q_ctx = _query_ctx_map.find(query_id)->second;
Review Comment:
q_ctx maybe not exist, for example, FE send duplicate cancel message to BE
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002458440
<details>
<summary>TPC-H: <b>Total hot run time: 38262 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 70581e0a54a974d39e500005f96f9cba48b23d20, data reload: false
------ Round 1 ----------------------------------
q1 17661 4412 4107 4107
q2 2027 155 149 149
q3 10583 1107 875 875
q4 7443 711 713 711
q5 7470 2730 2755 2730
q6 184 125 125 125
q7 1151 821 805 805
q8 9329 2019 2003 2003
q9 7171 6448 6381 6381
q10 8516 3444 3631 3444
q11 430 230 217 217
q12 638 294 296 294
q13 17793 2821 2839 2821
q14 295 242 259 242
q15 509 454 442 442
q16 522 399 391 391
q17 958 543 628 543
q18 7183 6439 6490 6439
q19 5143 1474 1399 1399
q20 540 296 284 284
q21 6238 3562 3625 3562
q22 344 330 298 298
Total cold run time: 112128 ms
Total hot run time: 38262 ms
----- Round 2, with runtime_filter_mode=off -----
q1 4129 4072 4074 4072
q2 313 238 228 228
q3 2937 2803 2862 2803
q4 1838 1572 1552 1552
q5 5228 5262 5257 5257
q6 196 116 119 116
q7 2237 1842 1834 1834
q8 3149 3371 3288 3288
q9 8536 8566 8543 8543
q10 3717 3634 3708 3634
q11 533 447 441 441
q12 716 541 563 541
q13 16892 2879 2855 2855
q14 277 243 262 243
q15 487 451 440 440
q16 445 400 404 400
q17 1736 1502 1463 1463
q18 7442 7213 7086 7086
q19 1621 1508 1561 1508
q20 1871 1690 1709 1690
q21 4729 4769 4744 4744
q22 504 436 465 436
Total cold run time: 69533 ms
Total hot run time: 53174 ms
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1527691150
##########
be/src/runtime/query_context.cpp:
##########
@@ -162,12 +166,13 @@ bool QueryContext::cancel(bool v, std::string msg, Status new_status, int fragme
set_ready_to_execute(true);
{
- std::lock_guard<std::mutex> plock(pipeline_lock);
for (auto& ctx : fragment_id_to_pipeline_ctx) {
if (fragment_id == ctx.first) {
continue;
}
- ctx.second->cancel(PPlanFragmentCancelReason::INTERNAL_ERROR, msg);
+ if (auto pipeline_ctx = ctx.second.lock()) {
Review Comment:
we need a lock to protect fragment_id_to_pipeline_ctx.
For example, one thread is exec_plan_fragment, it will use fragment mgr's lock and insert to the map;
and here, the cancel logic will iterate the map, it may core.
And we should not expose the structure fragment_id_to_pipeline_ctx to fragment mgr.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002865461
TeamCity be ut coverage result:
Function Coverage: 35.28% (8699/24658)
Line Coverage: 27.09% (71177/262759)
Region Coverage: 26.34% (36923/140173)
Branch Coverage: 23.25% (18883/81212)
Coverage Report: http://coverage.selectdb-in.cc/coverage/5e7e5c7d8d6505cb396444e0a039d2a9e0f1d8e6_5e7e5c7d8d6505cb396444e0a039d2a9e0f1d8e6/report/index.html
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1527798768
##########
be/src/runtime/fragment_mgr.cpp:
##########
@@ -773,6 +780,7 @@ std::string FragmentMgr::dump_pipeline_tasks() {
}
Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
Review Comment:
warning: function 'exec_plan_fragment' has cognitive complexity of 88 (threshold 50) [readability-function-cognitive-complexity]
```cpp
Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
^
```
<details>
<summary>Additional context</summary>
**be/src/runtime/fragment_mgr.cpp:791:** +1
```cpp
const bool enable_pipeline_x = params.query_options.__isset.enable_pipeline_x_engine &&
^
```
**be/src/runtime/fragment_mgr.cpp:793:** +1, including nesting penalty of 0, nesting level increased to 1
```cpp
if (enable_pipeline_x) {
^
```
**be/src/runtime/fragment_mgr.cpp:805:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (!prepare_st.ok()) {
^
```
**be/src/runtime/fragment_mgr.cpp:813:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
for (size_t i = 0; i < params.local_params.size(); i++) {
^
```
**be/src/runtime/fragment_mgr.cpp:815:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:815:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:818:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!i && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:818:** +1
```cpp
if (!i && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:825:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (iter != _pipeline_map.end()) {
^
```
**be/src/runtime/fragment_mgr.cpp:832:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:832:** +1
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:847:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (!query_ctx->fragment_id_to_pipeline_ctx.contains(params.fragment_id)) {
^
```
**be/src/runtime/fragment_mgr.cpp:854:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
RETURN_IF_ERROR(context->submit());
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:854:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
RETURN_IF_ERROR(context->submit());
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:856:** +1, nesting level increased to 1
```cpp
} else {
^
```
**be/src/runtime/fragment_mgr.cpp:857:** nesting level increased to 2
```cpp
auto pre_and_submit = [&](int i) {
^
```
**be/src/runtime/fragment_mgr.cpp:864:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (iter != _pipeline_map.end()) {
^
```
**be/src/runtime/fragment_mgr.cpp:872:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:872:** +1
```cpp
if (!params.__isset.need_wait_execution_trigger ||
^
```
**be/src/runtime/fragment_mgr.cpp:887:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (!prepare_st.ok()) {
^
```
**be/src/runtime/fragment_mgr.cpp:897:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:897:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:900:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (i == 0 && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:900:** +1
```cpp
if (i == 0 && handler) {
^
```
**be/src/runtime/fragment_mgr.cpp:915:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (local_params.__isset.runtime_filter_params) {
^
```
**be/src/runtime/fragment_mgr.cpp:916:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (local_params.__isset.runtime_filter_params) {
^
```
**be/src/runtime/fragment_mgr.cpp:921:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (local_params.__isset.topn_filter_source_node_ids) {
^
```
**be/src/runtime/fragment_mgr.cpp:923:** +1, nesting level increased to 2
```cpp
} else {
^
```
**be/src/runtime/fragment_mgr.cpp:927:** +2, including nesting penalty of 1, nesting level increased to 2
```cpp
if (target_size > 1) {
^
```
**be/src/runtime/fragment_mgr.cpp:933:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
for (size_t i = 0; i < target_size; i++) {
^
```
**be/src/runtime/fragment_mgr.cpp:934:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
^
```
**be/src/common/status.h:541:** expanded from macro 'RETURN_IF_ERROR'
```cpp
do { \
^
```
**be/src/runtime/fragment_mgr.cpp:934:** +5, including nesting penalty of 4, nesting level increased to 5
```cpp
RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
^
```
**be/src/common/status.h:543:** expanded from macro 'RETURN_IF_ERROR'
```cpp
if (UNLIKELY(!_status_.ok())) { \
^
```
**be/src/runtime/fragment_mgr.cpp:945:** +3, including nesting penalty of 2, nesting level increased to 3
```cpp
if (prepare_done != target_size) {
^
```
**be/src/runtime/fragment_mgr.cpp:948:** +4, including nesting penalty of 3, nesting level increased to 4
```cpp
for (size_t i = 0; i < target_size; i++) {
^
```
**be/src/runtime/fragment_mgr.cpp:949:** +5, including nesting penalty of 4, nesting level increased to 5
```cpp
if (!prepare_status[i].ok()) {
^
```
**be/src/runtime/fragment_mgr.cpp:955:** +1, nesting level increased to 2
```cpp
} else {
^
```
</details>
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "Gabriel39 (via GitHub)" <gi...@apache.org>.
Gabriel39 commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002739034
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1527691150
##########
be/src/runtime/query_context.cpp:
##########
@@ -162,12 +166,13 @@ bool QueryContext::cancel(bool v, std::string msg, Status new_status, int fragme
set_ready_to_execute(true);
{
- std::lock_guard<std::mutex> plock(pipeline_lock);
for (auto& ctx : fragment_id_to_pipeline_ctx) {
if (fragment_id == ctx.first) {
continue;
}
- ctx.second->cancel(PPlanFragmentCancelReason::INTERNAL_ERROR, msg);
+ if (auto pipeline_ctx = ctx.second.lock()) {
Review Comment:
we need a lock to protect fragment_id_to_pipeline_ctx.
For example, one thread is exec_plan_fragment, it will use fragment mgr's lock and insert to the map;
and here, the cancel logic will iterate the map, it may core.
And I think the core reason is we expose the structure fragment_id_to_pipeline_ctx to fragment mgr. We should change all members to priviate and expose some method to fragment mgr.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002728951
clang-tidy review says "All clean, LGTM! :+1:"
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "Gabriel39 (via GitHub)" <gi...@apache.org>.
Gabriel39 commented on PR #32346:
URL: https://github.com/apache/doris/pull/32346#issuecomment-2002724720
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [pipelineX](cancel) Remove lock for mapping query ctx to fragment [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei commented on code in PR #32346:
URL: https://github.com/apache/doris/pull/32346#discussion_r1527664578
##########
be/src/runtime/fragment_mgr.cpp:
##########
@@ -1047,7 +1038,7 @@ void FragmentMgr::cancel_fragment_unlocked(const TUniqueId& query_id, int32_t fr
auto q_ctx = _query_ctx_map.find(query_id)->second;
auto f_context = q_ctx->fragment_id_to_pipeline_ctx.find(fragment_id);
if (f_context != q_ctx->fragment_id_to_pipeline_ctx.end()) {
- f_context->second->cancel(reason, msg);
+ f_context->second.lock()->cancel(reason, msg);
Review Comment:
lock may fail
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org