Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fix](client) Do not log in thrift exception when ADDRESS_SANITIZER is defined #48347

Merged
merged 1 commit into from
Feb 26, 2025
Merged

[fix](client) Do not log in thrift exception when ADDRESS_SANITIZER is defined #48347

merged 1 commit into from
Feb 26, 2025
+7 −5

Conversation

w41ter
Copy link
Contributor

@w41ter w41ter commented Feb 26, 2025

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #36808

Problem Summary:

Logging in thrift exception will cause BE crash when ADDRESS_SANITIZER is defined. But PR #36808 uses the opposite condition, causing logging to be turned on only in ADDRESS_SANITIZER enabled.

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason

  • Behavior changed:

    • No.
    • Yes.

  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

36808

@Thearas
Copy link
Contributor

Thearas commented Feb 26, 2025

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See 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?

dataroaring
dataroaring approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@dataroaring dataroaring left a comment

Choose a reason for hiding this comment

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

LGTM

morningman
morningman approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions GitHub Actions
Copy link
Contributor

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

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Feb 26, 2025
@github-actions GitHub Actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@w41ter
Copy link
Contributor Author

w41ter commented Feb 26, 2025

run buildall

@doris-robot
Copy link

TPC-H: Total hot run time: 31986 ms 
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 8922fb0d7b5c04b04faddcbdd2b92ecc12e2f6bc, data reload: false

------ Round 1 ----------------------------------
q1	17595	5258	5059	5059
q2	2058	311	170	170
q3	10388	1263	786	786
q4	10204	1023	549	549
q5	7537	2470	2331	2331
q6	188	168	140	140
q7	926	754	625	625
q8	9325	1213	1109	1109
q9	4813	4650	4629	4629
q10	6825	2332	1940	1940
q11	483	275	253	253
q12	347	358	222	222
q13	17852	3896	3284	3284
q14	243	250	218	218
q15	507	474	474	474
q16	643	641	605	605
q17	585	860	347	347
q18	6918	6306	6220	6220
q19	1282	937	533	533
q20	312	327	192	192
q21	2817	2164	1995	1995
q22	367	331	305	305
Total cold run time: 102215 ms
Total hot run time: 31986 ms

----- Round 2, with runtime_filter_mode=off -----
q1	5129	5114	5081	5081
q2	242	334	230	230
q3	2202	2731	2280	2280
q4	1426	1785	1405	1405
q5	4253	4182	4171	4171
q6	205	162	124	124
q7	1886	1822	1720	1720
q8	2614	2716	2599	2599
q9	7161	7194	7117	7117
q10	3000	3220	2815	2815
q11	588	514	491	491
q12	685	776	633	633
q13	3386	3979	3353	3353
q14	277	319	270	270
q15	523	467	475	467
q16	648	672	677	672
q17	1126	1583	1343	1343
q18	7454	7421	7277	7277
q19	794	805	891	805
q20	1958	2058	1866	1866
q21	5486	5101	4727	4727
q22	679	568	541	541
Total cold run time: 51722 ms
Total hot run time: 49987 ms

@doris-robot
Copy link

TPC-DS: Total hot run time: 191375 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 8922fb0d7b5c04b04faddcbdd2b92ecc12e2f6bc, data reload: false

query1	1307	942	941	941
query2	6196	1858	1852	1852
query3	11121	4422	4347	4347
query4	56409	25836	23756	23756
query5	5551	551	500	500
query6	502	206	192	192
query7	6036	518	298	298
query8	332	264	254	254
query9	8633	2701	2676	2676
query10	432	306	262	262
query11	15691	15224	15164	15164
query12	165	120	123	120
query13	1328	538	440	440
query14	10757	6892	6710	6710
query15	209	190	192	190
query16	7097	657	491	491
query17	1105	724	595	595
query18	1513	418	322	322
query19	206	200	170	170
query20	127	126	121	121
query21	209	126	105	105
query22	4283	4518	4345	4345
query23	34094	33169	33359	33169
query24	5851	2475	2434	2434
query25	463	474	402	402
query26	750	284	159	159
query27	2011	502	368	368
query28	2814	2470	2454	2454
query29	617	591	444	444
query30	222	195	166	166
query31	934	916	798	798
query32	79	63	65	63
query33	486	365	313	313
query34	775	874	527	527
query35	837	870	773	773
query36	954	1028	914	914
query37	132	93	76	76
query38	4163	4278	4207	4207
query39	1477	1421	1443	1421
query40	213	116	103	103
query41	52	52	52	52
query42	124	106	112	106
query43	516	515	504	504
query44	1297	829	820	820
query45	189	180	171	171
query46	873	1053	668	668
query47	1815	1877	1777	1777
query48	401	450	342	342
query49	762	529	436	436
query50	720	762	417	417
query51	4303	4378	4251	4251
query52	106	101	100	100
query53	232	258	191	191
query54	489	499	421	421
query55	79	84	76	76
query56	276	292	274	274
query57	1186	1200	1113	1113
query58	261	258	246	246
query59	2817	2906	2655	2655
query60	308	275	273	273
query61	125	127	116	116
query62	775	731	678	678
query63	236	197	193	193
query64	2310	1051	695	695
query65	3241	3188	3148	3148
query66	840	396	299	299
query67	15762	15648	15383	15383
query68	6741	865	508	508
query69	552	293	258	258
query70	1192	1118	1110	1110
query71	477	305	269	269
query72	5751	3610	3727	3610
query73	1206	728	355	355
query74	9215	9272	8795	8795
query75	3643	3161	2712	2712
query76	4218	1176	746	746
query77	580	385	281	281
query78	9973	10236	9154	9154
query79	2350	824	587	587
query80	657	524	442	442
query81	497	286	246	246
query82	647	126	98	98
query83	179	168	149	149
query84	280	101	77	77
query85	804	352	299	299
query86	363	312	282	282
query87	4445	4710	4368	4368
query88	3373	2227	2205	2205
query89	411	322	286	286
query90	1917	191	197	191
query91	143	138	109	109
query92	72	63	56	56
query93	1211	1075	577	577
query94	688	416	308	308
query95	357	272	263	263
query96	490	552	271	271
query97	3344	3401	3274	3274
query98	228	212	204	204
query99	1683	1381	1238	1238
Total cold run time: 303863 ms
Total hot run time: 191375 ms

@doris-robot
Copy link

ClickBench: Total hot run time: 31.24 s 
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 8922fb0d7b5c04b04faddcbdd2b92ecc12e2f6bc, data reload: false

query1	0.04	0.03	0.03
query2	0.07	0.04	0.04
query3	0.23	0.07	0.06
query4	1.61	0.10	0.11
query5	0.56	0.53	0.54
query6	1.19	0.73	0.71
query7	0.02	0.02	0.02
query8	0.04	0.03	0.04
query9	0.59	0.55	0.52
query10	0.57	0.57	0.58
query11	0.16	0.11	0.11
query12	0.14	0.11	0.11
query13	0.61	0.60	0.58
query14	2.80	2.80	2.80
query15	0.90	0.85	0.84
query16	0.38	0.37	0.39
query17	0.99	1.02	1.02
query18	0.22	0.20	0.20
query19	1.85	1.78	1.97
query20	0.01	0.01	0.02
query21	15.35	0.88	0.52
query22	0.74	1.14	0.58
query23	15.07	1.36	0.62
query24	6.80	1.93	1.30
query25	0.53	0.25	0.21
query26	0.62	0.16	0.14
query27	0.05	0.05	0.05
query28	9.97	0.80	0.43
query29	12.64	3.92	3.28
query30	0.25	0.09	0.06
query31	2.82	0.58	0.38
query32	3.23	0.53	0.48
query33	3.09	3.05	3.00
query34	15.82	5.11	4.46
query35	4.53	4.48	4.52
query36	0.67	0.49	0.49
query37	0.08	0.06	0.06
query38	0.05	0.04	0.04
query39	0.04	0.02	0.02
query40	0.16	0.13	0.13
query41	0.08	0.03	0.03
query42	0.04	0.02	0.03
query43	0.04	0.03	0.03
Total cold run time: 105.65 s
Total hot run time: 31.24 s

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 44.76% (11933/26658)
Line Coverage: 34.21% (99877/291925)
Region Coverage: 33.36% (51108/153195)
Branch Coverage: 28.92% (25666/88762)
Coverage Report: http://coverage.selectdb-in.cc/coverage/8922fb0d7b5c04b04faddcbdd2b92ecc12e2f6bc_8922fb0d7b5c04b04faddcbdd2b92ecc12e2f6bc/report/index.html
Increment Report: http://coverage.selectdb-in.cc/coverage/8922fb0d7b5c04b04faddcbdd2b92ecc12e2f6bc_8922fb0d7b5c04b04faddcbdd2b92ecc12e2f6bc/increment_report/index.html

@hello-stephen
Copy link
Contributor

BE UT Coverage Report

Increment line coverage 0.00% (0/7) 🎉

Complete coverage report
Increment coverage report

Category Coverage
Function Coverage 44.75% (11930/26658)
Line Coverage 34.20% (99834/291925)
Region Coverage 33.36% (51100/153195)
Branch Coverage 28.91% (25661/88762)

@w41ter w41ter merged commit 87a9848 into apache:master Feb 26, 2025
29 of 30 checks passed
@w41ter w41ter deleted the fix_master_server_client_sanitizer_logs branch February 26, 2025 07:24
github-actions bot pushed a commit that referenced this pull request Feb 26, 2025
@w41ter
[fix](client) Do not log in thrift exception when ADDRESS_SANITIZER i…
6c8b50c 
…s defined (#48347)

Logging in thrift exception will cause BE crash when ADDRESS_SANITIZER
is defined. But PR #36808 uses the opposite condition, causing logging
to be turned on only in ADDRESS_SANITIZER enabled.
@github-actions github-actions bot mentioned this pull request Feb 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morningman morningman morningman approved these changes

@dataroaring dataroaring dataroaring approved these changes

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by one committer. dev/2.1.x dev/2.1.x-conflict dev/3.0.x p0_test reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@w41ter @Thearas @doris-robot @hello-stephen @morningman @dataroaring