Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
morrySnow commented on PR #63299: URL: https://github.com/apache/doris/pull/63299#issuecomment-4611848311 /review -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
hello-stephen commented on PR #63299: URL: https://github.com/apache/doris/pull/63299#issuecomment-4467045768 # FE Regression Coverage Report Increment line coverage ` 43.33% (13/30)` :tada: [Increment coverage report](http://coverage.selectdb-in.cc/coverage/63299_aa3a9619a518a52b3e6fd891d417be6539905d3e_merge_fe/increment_report/index.html) [Complete coverage report](http://coverage.selectdb-in.cc/coverage/63299_aa3a9619a518a52b3e6fd891d417be6539905d3e_merge_fe/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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
hello-stephen commented on PR #63299: URL: https://github.com/apache/doris/pull/63299#issuecomment-4467030210 # BE Regression && UT Coverage Report Increment line coverage `85.96% (49/57)` :tada: [Increment coverage report](http://coverage.selectdb-in.cc/coverage/63299_aa3a9619a518a52b3e6fd891d417be6539905d3e_merge/increment_report/index.html) [Complete coverage report](http://coverage.selectdb-in.cc/coverage/63299_aa3a9619a518a52b3e6fd891d417be6539905d3e_merge/report/index.html) | Category | Coverage | |---|| | Function Coverage | 73.68% (27843/37787) | | Line Coverage | 57.61% (301694/523705) | | Region Coverage | 54.83% (252109/459823) | | Branch Coverage | 56.36% (108980/193352) | -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
hello-stephen commented on PR #63299: URL: https://github.com/apache/doris/pull/63299#issuecomment-4466921739 # BE UT Coverage Report Increment line coverage `0.00% (0/57)` :tada: [Increment coverage report](http://coverage.selectdb-in.cc/coverage/aa3a9619a518a52b3e6fd891d417be6539905d3e_aa3a9619a518a52b3e6fd891d417be6539905d3e/increment_report/index.html) [Complete coverage report](http://coverage.selectdb-in.cc/coverage/aa3a9619a518a52b3e6fd891d417be6539905d3e_aa3a9619a518a52b3e6fd891d417be6539905d3e/report/index.html) | Category | Coverage | |---|| | Function Coverage | 53.49% (20640/38587) | | Line Coverage | 37.15% (195089/525082) | | Region Coverage | 33.55% (152799/455408) | | Branch Coverage | 34.57% (66580/192622) | -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
hello-stephen commented on PR #63299: URL: https://github.com/apache/doris/pull/63299#issuecomment-4466712389 TPC-DS: Total hot run time: 169145 ms ``` machine: 'aliyun_ecs.c7a.8xlarge_32C64G' scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools TPC-DS sf100 test result on commit aa3a9619a518a52b3e6fd891d417be6539905d3e, data reload: false query5 4328660 524 524 query6 343 227 201 201 query7 4229590 318 318 query8 324 224 209 209 query9 8849401240114011 query10 453 346 311 311 query11 5825237622042204 query12 193 133 131 131 query13 1278608 444 444 query14 6450535850715071 query14_14373433143234323 query15 216 205 179 179 query16 1039461 470 461 query17 1163776 609 609 query18 2716491 371 371 query19 226 220 167 167 query20 140 135 135 135 query21 216 141 121 121 query22 13618 13583 13414 13414 query23 17273 16364 16069 16069 query23_116196 16273 16173 16173 query24 7543177812851285 query24_11298129312801280 query25 548 479 419 419 query26 1326317 171 171 query27 2686537 347 347 query28 4327193419291929 query29 961 616 489 489 query30 308 237 201 201 query31 1065959 959 query32 89 75 73 73 query33 531 344 296 296 query34 1168639 639 query35 762 782 677 677 query36 1346133711731173 query37 152 102 91 91 query38 3215313530513051 query39 948 908 910 908 query39_1876 874 883 874 query40 232 150 127 127 query41 67 77 63 63 query42 110 111 111 111 query43 319 318 296 296 query44 query45 211 201 189 189 query46 11031219710 710 query47 2290239322312231 query48 362 430 298 298 query49 620 487 384 384 query50 1000338 255 255 query51 4276437942324232 query52 106 106 94 94 query53 249 279 203 203 query54 327 289 248 248 query55 94 91 85 85 query56 308 317 309 309 query57 1415140813261326 query58 300 274 271 271 query59 1534159014041404 query60 319 321 308 308 query61 156 156 150 150 query62 679 620 564 564 query63 244 208 202 202 query64 2367795 626 626 query65 query66 1651472 351 351 query67 30103 29979 29281 29281 query68 query69 465 343 305 305 query70 998 1058973 973 query71 313 273 271 271 query72 3057266323872387 query73 844 760 432 432 query74 5062490247354735 query75 2668260422262226 query76 23041129743 743 query77 387 413 333 333 query78 12189 12087 11693 11693 query79 14461017720 720 query80 663 558 448 448 query81 449 273 244 244 query82 1399156 123 123 query83 362 282 249 249 query84 267 141 107 107 query85 881 532 445 445 query86 386 348 329 329 query87 3406345032403240 query88 3500265726392639 query89 450 388 337 337 query90 1975179 183 179 query91 180 167 142 142 query92 82 80 76 76 query93 14931506898 898 query94 533 337 289 289 query95 679 398 448 398 query96 1044764 327 327 query97 2699270725652565 query98 237 231 230 230 query99 11001099997 997 Total cold run time: 253453 ms Total hot run time: 169145 ms ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
hello-stephen commented on PR #63299: URL: https://github.com/apache/doris/pull/63299#issuecomment-4466690580 TPC-H: Total hot run time: 31239 ms ``` machine: 'aliyun_ecs.c7a.8xlarge_32C64G' scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools Tpch sf100 test result on commit aa3a9619a518a52b3e6fd891d417be6539905d3e, data reload: false -- Round 1 -- orders Doris NULLNULL0 0 0 NULL0 NULLNULL2023-12-26 18:27:23 2023-12-26 18:42:55 NULLutf-8 NULLNULL q1 17665 393538813881 q2 q3 10776 1398823 823 q4 4687470 349 349 q5 7610223421152115 q6 242 176 142 142 q7 939 781 637 637 q8 9420176717031703 q9 5166498949734973 q10 6378207417981798 q11 442 277 253 253 q12 628 427 300 300 q13 18082 345627862786 q14 262 259 241 241 q15 q16 795 778 712 712 q17 936 889 1000889 q18 7227566855365536 q19 1159132911491149 q20 556 413 270 270 q21 5608262323762376 q22 438 371 306 306 Total cold run time: 99016 ms Total hot run time: 31239 ms - Round 2, with runtime_filter_mode=off - orders Doris NULLNULL15000 42 6422171781 NULL22778155NULLNULL2023-12-26 18:27:23 2023-12-26 18:42:55 NULLutf-8 NULLNULL q1 4180409341194093 q2 q3 4518494342724272 q4 2118220614221422 q5 4393431443064306 q6 231 184 130 130 q7 1964193917271727 q8 2571218721782178 q9 8012788775527552 q10 4560453040594059 q11 604 414 377 377 q12 726 747 516 516 q13 3293362429502950 q14 290 292 272 272 q15 q16 713 735 644 644 q17 1347131513301315 q18 8053733871977197 q19 1187114211341134 q20 2200220419291929 q21 5301462344974497 q22 530 480 428 428 Total cold run time: 56791 ms Total hot run time: 50998 ms ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
hello-stephen commented on PR #63299: URL: https://github.com/apache/doris/pull/63299#issuecomment-4466581100 # Cloud UT Coverage Report Increment line coverage ` ` :tada: [Increment coverage report](http://coverage.selectdb-in.cc/coverage/aa3a9619a518a52b3e6fd891d417be6539905d3e_aa3a9619a518a52b3e6fd891d417be6539905d3e_cloud/increment_report/index.html) [Complete coverage report](http://coverage.selectdb-in.cc/coverage/aa3a9619a518a52b3e6fd891d417be6539905d3e_aa3a9619a518a52b3e6fd891d417be6539905d3e_cloud/report/index.html) | Category | Coverage | |---|| | Function Coverage | 78.06% (1854/2375) | | Line Coverage | 64.52% (33329/51653) | | Region Coverage | 65.23% (16527/25335) | | Branch Coverage | 55.71% (8829/15848) | -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
foxtail463 commented on code in PR #63299: URL: https://github.com/apache/doris/pull/63299#discussion_r3252612248 ## be/src/runtime/runtime_query_statistics_mgr.cpp: ## @@ -230,8 +219,7 @@ TReportExecStatusParams RuntimeQueryStatisticsMgr::create_report_exec_status_par TReportExecStatusParams req; THRIFT_MOVE_VALUES(req, query_profile, profile); req.__set_backend_id(ExecEnv::GetInstance()->cluster_info()->backend_id); -// invalid query id to avoid API compatibility during upgrade -req.__set_query_id(TUniqueId()); +req.__set_query_id(query_id); Review Comment: fix -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
foxtail463 commented on PR #63299: URL: https://github.com/apache/doris/pull/63299#issuecomment-4466488581 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
foxtail463 commented on code in PR #63299:
URL: https://github.com/apache/doris/pull/63299#discussion_r3252613012
##
fe/fe-core/src/main/java/org/apache/doris/common/profile/ExecutionProfile.java:
##
@@ -224,48 +225,56 @@ public Status updateProfile(TQueryProfile profile,
TNetworkAddress backendHBAddr
return new Status(TStatusCode.INVALID_ARGUMENT, "QueryId is not
set");
}
-if (!profile.isSetFragmentIdToProfile()) {
-LOG.warn("{} FragmentIdToProfile is not set",
DebugUtil.printId(profile.getQueryId()));
-return new Status(TStatusCode.INVALID_ARGUMENT,
"FragmentIdToProfile is not set");
+if (!profile.isSetFragmentIdToProfileNodeReports()) {
Review Comment:
The impact is limited to profile visibility during rolling
upgrade. Query execution status, load progress and correctness are not
affected.
Since this report path is only for profile collection and the new structured
field is
the intended format after decoupling profile node reports from the display
tree, I
would prefer not to add legacy fallback
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
github-actions[bot] commented on code in PR #63299:
URL: https://github.com/apache/doris/pull/63299#discussion_r3247539401
##
be/src/runtime/runtime_query_statistics_mgr.cpp:
##
@@ -230,8 +219,7 @@ TReportExecStatusParams
RuntimeQueryStatisticsMgr::create_report_exec_status_par
TReportExecStatusParams req;
THRIFT_MOVE_VALUES(req, query_profile, profile);
req.__set_backend_id(ExecEnv::GetInstance()->cluster_info()->backend_id);
-// invalid query id to avoid API compatibility during upgrade
-req.__set_query_id(TUniqueId());
+req.__set_query_id(query_id);
Review Comment:
This breaks the mixed-version profile-reporting contract. Before this change
the profile-only RPC deliberately sent an invalid `query_id` (the removed
comment says it was to avoid API compatibility issues during upgrade), so an
older FE would not route this profile-only request into normal
`updateFragmentExecStatus`. Now a new BE sends the real query id while omitting
`status`, and an old FE that does not have the new early return can find the
coordinator and process a status-less exec report, while also rejecting
`query_profile` because it only understands `fragment_id_to_profile`. Please
keep the old compatibility behavior or add a version-compatible path before
changing this field.
##
fe/fe-core/src/main/java/org/apache/doris/common/profile/ExecutionProfile.java:
##
@@ -224,48 +225,56 @@ public Status updateProfile(TQueryProfile profile,
TNetworkAddress backendHBAddr
return new Status(TStatusCode.INVALID_ARGUMENT, "QueryId is not
set");
}
-if (!profile.isSetFragmentIdToProfile()) {
-LOG.warn("{} FragmentIdToProfile is not set",
DebugUtil.printId(profile.getQueryId()));
-return new Status(TStatusCode.INVALID_ARGUMENT,
"FragmentIdToProfile is not set");
+if (!profile.isSetFragmentIdToProfileNodeReports()) {
Review Comment:
This makes a new FE reject profile reports from old BEs during a rolling
upgrade. The thrift struct still has the legacy `fragment_id_to_profile` field,
and old BEs will continue to send only that field until they are upgraded; with
this guard, `updateProfile()` returns `INVALID_ARGUMENT` and drops those
profiles instead of translating the old `TDetailedReportParams` format. Please
keep a fallback for `fragment_id_to_profile` until mixed-version reporting is
no longer supported.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
morrySnow commented on PR #63299: URL: https://github.com/apache/doris/pull/63299#issuecomment-4458963551 /review -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [refactor](profile) Decouple profile node reports from display tree [doris]
hello-stephen commented on PR #63299: URL: https://github.com/apache/doris/pull/63299#issuecomment-4458931682 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). Please clearly describe your PR: 1. What problem was fixed (it's best to include specific error reporting information). How it was fixed. 2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be. 3. What features were added. Why was this function added? 4. Which code was refactored and why was this part of the code refactored? 5. Which functions were optimized and what is the difference before and after the optimization? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
