Skip to content

[pipelineX](cancel) Remove lock for mapping query ctx to fragment#32346

Merged
yiguolei merged 11 commits intoapache:masterfrom
Gabriel39:dev_0317
Mar 18, 2024
Merged

[pipelineX](cancel) Remove lock for mapping query ctx to fragment#32346
yiguolei merged 11 commits intoapache:masterfrom
Gabriel39:dev_0317

Conversation

@Gabriel39
Copy link
Copy Markdown
Contributor

Proposed changes

Issue Number: close #xxx

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@doris-robot
Copy link
Copy Markdown

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

@Gabriel39
Copy link
Copy Markdown
Contributor Author

run buildall

@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@doris-robot
Copy link
Copy Markdown

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

@doris-robot
Copy link
Copy Markdown

TPC-H: Total hot run time: 38262 ms
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

Comment thread be/src/runtime/query_context.cpp Outdated
Comment thread be/src/runtime/fragment_mgr.cpp Outdated
Comment thread be/src/runtime/fragment_mgr.cpp Outdated
Comment thread be/src/runtime/fragment_mgr.cpp Outdated
@Gabriel39
Copy link
Copy Markdown
Contributor Author

run buildall

@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@Gabriel39
Copy link
Copy Markdown
Contributor Author

run buildall

@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@Gabriel39
Copy link
Copy Markdown
Contributor Author

run buildall

1 similar comment
@Gabriel39
Copy link
Copy Markdown
Contributor Author

run buildall

@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Comment thread be/src/runtime/query_context.cpp Outdated
continue;
}
ctx.second->cancel(PPlanFragmentCancelReason::INTERNAL_ERROR, msg);
if (auto pipeline_ctx = ctx.second.lock()) {
Copy link
Copy Markdown
Contributor

@yiguolei yiguolei Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread be/src/runtime/fragment_mgr.cpp Outdated
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()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@doris-robot
Copy link
Copy Markdown

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

@Gabriel39
Copy link
Copy Markdown
Contributor Author

run buildall

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment thread be/src/runtime/fragment_mgr.cpp Outdated
}
}

Status FragmentMgr::get_query_ctx(const TPipelineFragmentParams& params, TUniqueId query_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'get_query_ctx' can be made static [readability-convert-member-functions-to-static]

Suggested change
Status FragmentMgr::get_query_ctx(const TPipelineFragmentParams& params, TUniqueId query_id,
static Status FragmentMgr::get_query_ctx(const TPipelineFragmentParams& params, TUniqueId query_id,

@@ -773,6 +779,7 @@ std::string FragmentMgr::dump_pipeline_tasks() {
}

Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'exec_plan_fragment' has cognitive complexity of 88 (threshold 50) [readability-function-cognitive-complexity]

Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
                    ^
Additional context

be/src/runtime/fragment_mgr.cpp:790: +1

    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

    if (enable_pipeline_x) {
    ^

be/src/runtime/fragment_mgr.cpp:804: +2, including nesting penalty of 1, nesting level increased to 2

            if (!prepare_st.ok()) {
            ^

be/src/runtime/fragment_mgr.cpp:812: +2, including nesting penalty of 1, nesting level increased to 2

        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

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:814: +4, including nesting penalty of 3, nesting level increased to 4

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:817: +3, including nesting penalty of 2, nesting level increased to 3

            if (!i && handler) {
            ^

be/src/runtime/fragment_mgr.cpp:817: +1

            if (!i && handler) {
                   ^

be/src/runtime/fragment_mgr.cpp:824: +3, including nesting penalty of 2, nesting level increased to 3

                if (iter != _pipeline_map.end()) {
                ^

be/src/runtime/fragment_mgr.cpp:831: +3, including nesting penalty of 2, nesting level increased to 3

            if (!params.__isset.need_wait_execution_trigger ||
            ^

be/src/runtime/fragment_mgr.cpp:831: +1

            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

            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

        RETURN_IF_ERROR(context->submit());
        ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:853: +3, including nesting penalty of 2, nesting level increased to 3

        RETURN_IF_ERROR(context->submit());
        ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:855: +1, nesting level increased to 1

    } else {
      ^

be/src/runtime/fragment_mgr.cpp:856: nesting level increased to 2

        auto pre_and_submit = [&](int i) {
                              ^

be/src/runtime/fragment_mgr.cpp:863: +3, including nesting penalty of 2, nesting level increased to 3

                if (iter != _pipeline_map.end()) {
                ^

be/src/runtime/fragment_mgr.cpp:871: +3, including nesting penalty of 2, nesting level increased to 3

            if (!params.__isset.need_wait_execution_trigger ||
            ^

be/src/runtime/fragment_mgr.cpp:871: +1

            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

                if (!prepare_st.ok()) {
                ^

be/src/runtime/fragment_mgr.cpp:896: +3, including nesting penalty of 2, nesting level increased to 3

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:896: +4, including nesting penalty of 3, nesting level increased to 4

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:899: +3, including nesting penalty of 2, nesting level increased to 3

            if (i == 0 && handler) {
            ^

be/src/runtime/fragment_mgr.cpp:899: +1

            if (i == 0 && handler) {
                       ^

be/src/runtime/fragment_mgr.cpp:914: +2, including nesting penalty of 1, nesting level increased to 2

        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

            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

        if (local_params.__isset.topn_filter_source_node_ids) {
        ^

be/src/runtime/fragment_mgr.cpp:922: +1, nesting level increased to 2

        } else {
          ^

be/src/runtime/fragment_mgr.cpp:926: +2, including nesting penalty of 1, nesting level increased to 2

        if (target_size > 1) {
        ^

be/src/runtime/fragment_mgr.cpp:932: +3, including nesting penalty of 2, nesting level increased to 3

            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

                RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
                ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:933: +5, including nesting penalty of 4, nesting level increased to 5

                RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
                ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:944: +3, including nesting penalty of 2, nesting level increased to 3

            if (prepare_done != target_size) {
            ^

be/src/runtime/fragment_mgr.cpp:947: +4, including nesting penalty of 3, nesting level increased to 4

                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

                    if (!prepare_status[i].ok()) {
                    ^

be/src/runtime/fragment_mgr.cpp:954: +1, nesting level increased to 2

        } else {
          ^

@Gabriel39
Copy link
Copy Markdown
Contributor Author

run buildall

@Gabriel39
Copy link
Copy Markdown
Contributor Author

run buildall

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

@@ -773,6 +780,7 @@ std::string FragmentMgr::dump_pipeline_tasks() {
}

Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'exec_plan_fragment' has cognitive complexity of 88 (threshold 50) [readability-function-cognitive-complexity]

Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
                    ^
Additional context

be/src/runtime/fragment_mgr.cpp:791: +1

    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

    if (enable_pipeline_x) {
    ^

be/src/runtime/fragment_mgr.cpp:805: +2, including nesting penalty of 1, nesting level increased to 2

            if (!prepare_st.ok()) {
            ^

be/src/runtime/fragment_mgr.cpp:813: +2, including nesting penalty of 1, nesting level increased to 2

        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

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:815: +4, including nesting penalty of 3, nesting level increased to 4

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:818: +3, including nesting penalty of 2, nesting level increased to 3

            if (!i && handler) {
            ^

be/src/runtime/fragment_mgr.cpp:818: +1

            if (!i && handler) {
                   ^

be/src/runtime/fragment_mgr.cpp:825: +3, including nesting penalty of 2, nesting level increased to 3

                if (iter != _pipeline_map.end()) {
                ^

be/src/runtime/fragment_mgr.cpp:832: +3, including nesting penalty of 2, nesting level increased to 3

            if (!params.__isset.need_wait_execution_trigger ||
            ^

be/src/runtime/fragment_mgr.cpp:832: +1

            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

            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

        RETURN_IF_ERROR(context->submit());
        ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:854: +3, including nesting penalty of 2, nesting level increased to 3

        RETURN_IF_ERROR(context->submit());
        ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:856: +1, nesting level increased to 1

    } else {
      ^

be/src/runtime/fragment_mgr.cpp:857: nesting level increased to 2

        auto pre_and_submit = [&](int i) {
                              ^

be/src/runtime/fragment_mgr.cpp:864: +3, including nesting penalty of 2, nesting level increased to 3

                if (iter != _pipeline_map.end()) {
                ^

be/src/runtime/fragment_mgr.cpp:872: +3, including nesting penalty of 2, nesting level increased to 3

            if (!params.__isset.need_wait_execution_trigger ||
            ^

be/src/runtime/fragment_mgr.cpp:872: +1

            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

                if (!prepare_st.ok()) {
                ^

be/src/runtime/fragment_mgr.cpp:897: +3, including nesting penalty of 2, nesting level increased to 3

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:897: +4, including nesting penalty of 3, nesting level increased to 4

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:900: +3, including nesting penalty of 2, nesting level increased to 3

            if (i == 0 && handler) {
            ^

be/src/runtime/fragment_mgr.cpp:900: +1

            if (i == 0 && handler) {
                       ^

be/src/runtime/fragment_mgr.cpp:915: +2, including nesting penalty of 1, nesting level increased to 2

        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

            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

        if (local_params.__isset.topn_filter_source_node_ids) {
        ^

be/src/runtime/fragment_mgr.cpp:923: +1, nesting level increased to 2

        } else {
          ^

be/src/runtime/fragment_mgr.cpp:927: +2, including nesting penalty of 1, nesting level increased to 2

        if (target_size > 1) {
        ^

be/src/runtime/fragment_mgr.cpp:933: +3, including nesting penalty of 2, nesting level increased to 3

            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

                RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
                ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:934: +5, including nesting penalty of 4, nesting level increased to 5

                RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
                ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:945: +3, including nesting penalty of 2, nesting level increased to 3

            if (prepare_done != target_size) {
            ^

be/src/runtime/fragment_mgr.cpp:948: +4, including nesting penalty of 3, nesting level increased to 4

                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

                    if (!prepare_status[i].ok()) {
                    ^

be/src/runtime/fragment_mgr.cpp:955: +1, nesting level increased to 2

        } else {
          ^

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

@@ -773,6 +775,7 @@ std::string FragmentMgr::dump_pipeline_tasks() {
}

Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'exec_plan_fragment' has cognitive complexity of 91 (threshold 50) [readability-function-cognitive-complexity]

Status FragmentMgr::exec_plan_fragment(const TPipelineFragmentParams& params,
                    ^
Additional context

be/src/runtime/fragment_mgr.cpp:786: +1

    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

    if (enable_pipeline_x) {
    ^

be/src/runtime/fragment_mgr.cpp:800: +2, including nesting penalty of 1, nesting level increased to 2

            if (!prepare_st.ok()) {
            ^

be/src/runtime/fragment_mgr.cpp:808: +2, including nesting penalty of 1, nesting level increased to 2

        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

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:810: +4, including nesting penalty of 3, nesting level increased to 4

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:813: +3, including nesting penalty of 2, nesting level increased to 3

            if (!i && handler) {
            ^

be/src/runtime/fragment_mgr.cpp:813: +1

            if (!i && handler) {
                   ^

be/src/runtime/fragment_mgr.cpp:820: +3, including nesting penalty of 2, nesting level increased to 3

                if (iter != _pipeline_map.end()) {
                ^

be/src/runtime/fragment_mgr.cpp:827: +3, including nesting penalty of 2, nesting level increased to 3

            if (!params.__isset.need_wait_execution_trigger ||
            ^

be/src/runtime/fragment_mgr.cpp:827: +1

            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

        RETURN_IF_ERROR(query_ctx->map_pipeline_context(
        ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:842: +3, including nesting penalty of 2, nesting level increased to 3

        RETURN_IF_ERROR(query_ctx->map_pipeline_context(
        ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:849: +2, including nesting penalty of 1, nesting level increased to 2

        RETURN_IF_ERROR(context->submit());
        ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:849: +3, including nesting penalty of 2, nesting level increased to 3

        RETURN_IF_ERROR(context->submit());
        ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:851: +1, nesting level increased to 1

    } else {
      ^

be/src/runtime/fragment_mgr.cpp:852: nesting level increased to 2

        auto pre_and_submit = [&](int i) {
                              ^

be/src/runtime/fragment_mgr.cpp:859: +3, including nesting penalty of 2, nesting level increased to 3

                if (iter != _pipeline_map.end()) {
                ^

be/src/runtime/fragment_mgr.cpp:867: +3, including nesting penalty of 2, nesting level increased to 3

            if (!params.__isset.need_wait_execution_trigger ||
            ^

be/src/runtime/fragment_mgr.cpp:867: +1

            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

                if (!prepare_st.ok()) {
                ^

be/src/runtime/fragment_mgr.cpp:892: +3, including nesting penalty of 2, nesting level increased to 3

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:892: +4, including nesting penalty of 3, nesting level increased to 4

            RETURN_IF_ERROR(_runtimefilter_controller.add_entity(
            ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:895: +3, including nesting penalty of 2, nesting level increased to 3

            if (i == 0 && handler) {
            ^

be/src/runtime/fragment_mgr.cpp:895: +1

            if (i == 0 && handler) {
                       ^

be/src/runtime/fragment_mgr.cpp:910: +2, including nesting penalty of 1, nesting level increased to 2

        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

            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

        if (local_params.__isset.topn_filter_source_node_ids) {
        ^

be/src/runtime/fragment_mgr.cpp:918: +1, nesting level increased to 2

        } else {
          ^

be/src/runtime/fragment_mgr.cpp:922: +2, including nesting penalty of 1, nesting level increased to 2

        if (target_size > 1) {
        ^

be/src/runtime/fragment_mgr.cpp:928: +3, including nesting penalty of 2, nesting level increased to 3

            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

                RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
                ^

be/src/common/status.h:541: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/runtime/fragment_mgr.cpp:929: +5, including nesting penalty of 4, nesting level increased to 5

                RETURN_IF_ERROR(_thread_pool->submit_func([&, i]() {
                ^

be/src/common/status.h:543: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/runtime/fragment_mgr.cpp:940: +3, including nesting penalty of 2, nesting level increased to 3

            if (prepare_done != target_size) {
            ^

be/src/runtime/fragment_mgr.cpp:943: +4, including nesting penalty of 3, nesting level increased to 4

                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

                    if (!prepare_status[i].ok()) {
                    ^

be/src/runtime/fragment_mgr.cpp:950: +1, nesting level increased to 2

        } else {
          ^

Comment thread be/src/runtime/query_context.cpp Outdated
Comment thread be/src/runtime/query_context.h Outdated
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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Suggested change
Status map_pipeline_context(MaphFunc map_func, const int fragment_id);
Status map_pipeline_context(MaphFunc map_func, int fragment_id);

// 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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use global static variable, we do not know when it is deconstruted.

Comment thread be/src/runtime/query_context.h Outdated
};

struct PipelineFragmentContextWrapper {
bool valid = false;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if valid variable is useless, then the wrapper is useless

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

}
}

Status QueryContext::cancel_pipeline_context(const int fragment_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'cancel_pipeline_context' can be made static [readability-convert-member-functions-to-static]

Suggested change
Status QueryContext::cancel_pipeline_context(const int fragment_id,
static Status QueryContext::cancel_pipeline_context(const int fragment_id,


void cancel_all_pipeline_context(const PPlanFragmentCancelReason& reason,
const std::string& msg);
Status cancel_pipeline_context(const int fragment_id, const PPlanFragmentCancelReason& reason,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Suggested change
Status cancel_pipeline_context(const int fragment_id, const PPlanFragmentCancelReason& reason,
Status cancel_pipeline_context(int fragment_id, 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,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Suggested change
void set_pipeline_context(const int fragment_id,
void set_pipeline_context(int fragment_id,

@Gabriel39
Copy link
Copy Markdown
Contributor Author

run buildall

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Mar 18, 2024
@github-actions
Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@doris-robot
Copy link
Copy Markdown

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants