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.

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

[fix](s3) fix invalid s3 properties checking logic #35762

Merged
merged 11 commits into from
Jun 3, 2024

Conversation

morningman
Copy link
Contributor

@morningman morningman commented Jun 1, 2024

Proposed changes

Introduced from #35515

  1. Fix invalid to_int() method logic
  2. Remove unnecessary properties when creating s3 resource
    Before, after recreating s3 resource, there will be some extra properties being added to the resource properties,
    such as AWS_ACCESS_KEY, but this keys are only for s3 client on BE side, don' t needed when ping s3.
    But it will add some invalid properties such as AWS_TOKEN=null

@doris-robot
Copy link

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

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

Copy link
Contributor

github-actions bot commented Jun 1, 2024

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

@morningman morningman changed the title [fix](s3) fix invalid s3 property check [fix](s3) fix invalid s3 properties checking logic Jun 1, 2024
Copy link
Contributor

github-actions bot commented Jun 1, 2024

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

1 similar comment
Copy link
Contributor

github-actions bot commented Jun 1, 2024

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

@morningman
Copy link
Contributor Author

run buildall

Copy link
Contributor

github-actions bot commented Jun 1, 2024

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

1 similar comment
Copy link
Contributor

github-actions bot commented Jun 1, 2024

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

morningman added a commit that referenced this pull request Jun 1, 2024
@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 36.27% (9235/25461)
Line Coverage: 27.62% (75746/274284)
Region Coverage: 26.84% (39213/146120)
Branch Coverage: 23.59% (19900/84366)
Coverage Report: http://coverage.selectdb-in.cc/coverage/2ff7298b862f3454f4393172cb1291e0998e5997_2ff7298b862f3454f4393172cb1291e0998e5997/report/index.html

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17605	4380	4295	4295
q2	2042	201	208	201
q3	10440	1200	1283	1200
q4	10187	909	781	781
q5	7539	2728	2778	2728
q6	225	142	144	142
q7	990	621	630	621
q8	9224	2167	2123	2123
q9	9261	6760	6784	6760
q10	9369	3908	3889	3889
q11	456	250	246	246
q12	445	250	253	250
q13	18607	3170	3259	3170
q14	278	223	223	223
q15	516	464	480	464
q16	516	388	390	388
q17	1025	601	657	601
q18	8478	7885	7776	7776
q19	5673	1533	1534	1533
q20	671	337	321	321
q21	5122	3986	3340	3340
q22	402	351	328	328
Total cold run time: 119071 ms
Total hot run time: 41380 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4497	4444	4451	4444
q2	378	267	280	267
q3	3153	2940	2934	2934
q4	2020	1666	1574	1574
q5	5377	5531	5484	5484
q6	238	122	128	122
q7	2230	1847	1815	1815
q8	3252	3346	3349	3346
q9	8744	8686	8741	8686
q10	4122	3885	3861	3861
q11	595	533	499	499
q12	780	604	639	604
q13	16892	3167	3157	3157
q14	312	275	282	275
q15	523	488	491	488
q16	485	459	426	426
q17	1832	1542	1470	1470
q18	7945	7784	7359	7359
q19	1761	1610	1657	1610
q20	3020	1788	1808	1788
q21	8249	4612	4741	4612
q22	588	518	526	518
Total cold run time: 76993 ms
Total hot run time: 55339 ms

kaka11chen
kaka11chen previously approved these changes Jun 1, 2024
Copy link
Contributor

@kaka11chen kaka11chen 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 bot added the approved Indicates a PR has been approved by one committer. label Jun 1, 2024
Copy link
Contributor

github-actions bot commented Jun 1, 2024

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

Copy link
Contributor

github-actions bot commented Jun 1, 2024

PR approved by anyone and no changes requested.

platoneko
platoneko previously approved these changes Jun 2, 2024
@morningman
Copy link
Contributor Author

run buildall

@github-actions github-actions bot removed the approved Indicates a PR has been approved by one committer. label Jun 3, 2024
Copy link
Contributor

github-actions bot commented Jun 3, 2024

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

@morningman
Copy link
Contributor Author

run buildall

Copy link
Contributor

github-actions bot commented Jun 3, 2024

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

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 36.26% (9232/25460)
Line Coverage: 27.60% (75687/274206)
Region Coverage: 26.82% (39200/146151)
Branch Coverage: 23.54% (19854/84346)
Coverage Report: http://coverage.selectdb-in.cc/coverage/eedd09c7d3b29c99bd78ba47685730beff83593f_eedd09c7d3b29c99bd78ba47685730beff83593f/report/index.html

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17608	4301	4245	4245
q2	2029	196	188	188
q3	10490	1194	1203	1194
q4	10150	840	869	840
q5	7689	2731	2708	2708
q6	222	135	137	135
q7	958	611	605	605
q8	9431	2085	2283	2085
q9	8975	6454	6440	6440
q10	8943	3691	3700	3691
q11	475	248	242	242
q12	409	228	223	223
q13	18734	2987	2982	2982
q14	261	215	218	215
q15	496	479	471	471
q16	517	390	384	384
q17	956	667	712	667
q18	7970	7473	7449	7449
q19	1939	1510	1448	1448
q20	648	309	309	309
q21	4928	3909	3869	3869
q22	398	352	348	348
Total cold run time: 114226 ms
Total hot run time: 40738 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4331	4218	4210	4210
q2	381	279	263	263
q3	2987	2676	2710	2676
q4	1850	1640	1627	1627
q5	5252	5246	5272	5246
q6	210	128	126	126
q7	2083	1679	1780	1679
q8	3210	3392	3327	3327
q9	8334	8326	8323	8323
q10	3821	3641	3685	3641
q11	577	501	490	490
q12	754	632	598	598
q13	16656	3021	3025	3021
q14	298	255	266	255
q15	520	481	482	481
q16	505	421	431	421
q17	1766	1493	1461	1461
q18	7627	7666	7457	7457
q19	6473	1556	1644	1556
q20	2013	1755	1776	1755
q21	4961	4710	4799	4710
q22	637	551	549	549
Total cold run time: 75246 ms
Total hot run time: 53872 ms

@doris-robot
Copy link

TPC-DS: Total hot run time: 173118 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 eedd09c7d3b29c99bd78ba47685730beff83593f, data reload: false

query1	932	394	385	385
query2	6444	2320	2407	2320
query3	6651	205	211	205
query4	19571	17405	17483	17405
query5	4147	471	451	451
query6	258	157	175	157
query7	4586	291	292	291
query8	320	307	293	293
query9	8594	2376	2365	2365
query10	455	306	285	285
query11	10451	10204	10054	10054
query12	141	97	88	88
query13	1663	369	361	361
query14	9567	7340	7449	7340
query15	257	179	182	179
query16	7925	267	277	267
query17	1825	524	530	524
query18	2004	271	271	271
query19	215	152	153	152
query20	95	87	84	84
query21	211	126	125	125
query22	4229	4182	4055	4055
query23	33620	33015	33201	33015
query24	12012	2842	2771	2771
query25	656	357	359	357
query26	1698	156	156	156
query27	2971	335	325	325
query28	7289	2025	2064	2025
query29	1022	605	606	605
query30	281	148	147	147
query31	945	720	755	720
query32	95	53	53	53
query33	762	281	275	275
query34	963	463	462	462
query35	766	602	605	602
query36	1113	981	960	960
query37	152	68	67	67
query38	2876	2808	2736	2736
query39	847	798	790	790
query40	284	125	129	125
query41	56	55	57	55
query42	116	95	97	95
query43	571	571	538	538
query44	1274	748	770	748
query45	204	171	169	169
query46	1090	713	724	713
query47	1857	1781	1793	1781
query48	394	311	332	311
query49	1146	404	404	404
query50	773	388	391	388
query51	7010	6820	6670	6670
query52	107	89	89	89
query53	356	304	284	284
query54	1019	454	440	440
query55	76	75	71	71
query56	270	248	268	248
query57	1182	1048	1048	1048
query58	266	258	245	245
query59	3303	3129	3219	3129
query60	329	264	263	263
query61	88	90	89	89
query62	675	448	447	447
query63	312	288	287	287
query64	9874	2213	1795	1795
query65	3162	3080	3097	3080
query66	1388	325	336	325
query67	15412	14860	14876	14860
query68	6189	562	548	548
query69	614	471	410	410
query70	1184	1147	1135	1135
query71	454	281	282	281
query72	7181	5444	5414	5414
query73	779	331	318	318
query74	5927	5422	5515	5422
query75	3765	2633	2660	2633
query76	3363	965	879	879
query77	639	294	295	294
query78	10313	9746	9725	9725
query79	2258	513	516	513
query80	1110	461	470	461
query81	534	277	217	217
query82	1136	100	98	98
query83	195	168	165	165
query84	270	87	83	83
query85	1389	270	280	270
query86	468	329	315	315
query87	3235	3095	3068	3068
query88	4066	2433	2427	2427
query89	460	389	380	380
query90	1819	183	184	183
query91	121	99	96	96
query92	63	50	49	49
query93	2307	519	499	499
query94	1174	193	188	188
query95	408	309	306	306
query96	597	268	263	263
query97	3233	3048	3003	3003
query98	243	229	222	222
query99	1216	859	833	833
Total cold run time: 277913 ms
Total hot run time: 173118 ms

@doris-robot
Copy link

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

query1	0.04	0.03	0.03
query2	0.08	0.04	0.03
query3	0.23	0.05	0.05
query4	1.67	0.08	0.09
query5	0.52	0.50	0.50
query6	1.14	0.72	0.72
query7	0.02	0.01	0.01
query8	0.05	0.04	0.04
query9	0.54	0.48	0.49
query10	0.56	0.56	0.54
query11	0.15	0.12	0.11
query12	0.14	0.12	0.13
query13	0.59	0.59	0.59
query14	0.76	0.79	0.78
query15	0.83	0.82	0.81
query16	0.36	0.36	0.37
query17	1.02	1.03	0.98
query18	0.21	0.27	0.23
query19	1.83	1.68	1.73
query20	0.01	0.02	0.01
query21	15.58	0.68	0.66
query22	4.11	7.51	2.07
query23	18.27	1.37	1.24
query24	1.83	0.25	0.23
query25	0.14	0.10	0.08
query26	0.27	0.17	0.17
query27	0.08	0.08	0.08
query28	13.29	1.01	0.98
query29	13.05	3.26	3.25
query30	0.25	0.06	0.05
query31	2.89	0.39	0.38
query32	3.27	0.49	0.48
query33	2.88	2.90	2.98
query34	17.00	4.40	4.42
query35	4.52	4.52	4.46
query36	0.65	0.48	0.46
query37	0.18	0.15	0.15
query38	0.15	0.15	0.15
query39	0.04	0.04	0.04
query40	0.17	0.14	0.15
query41	0.09	0.05	0.04
query42	0.06	0.05	0.05
query43	0.04	0.04	0.04
Total cold run time: 109.56 s
Total hot run time: 30.65 s

Copy link
Contributor

@kaka11chen kaka11chen 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 bot added the approved Indicates a PR has been approved by one committer. label Jun 3, 2024
Copy link
Contributor

github-actions bot commented Jun 3, 2024

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

@morningman morningman merged commit 9147c04 into apache:master Jun 3, 2024
26 of 29 checks passed
dataroaring pushed a commit that referenced this pull request Jun 4, 2024
## Proposed changes
Introduced from #35515

1. Fix invalid `to_int()` method logic
2. Remove unnecessary properties when creating s3 resource
Before, after recreating s3 resource, there will be some extra
properties being added to the resource properties,
such as AWS_ACCESS_KEY, but this keys are only for s3 client on BE side,
don' t needed when ping s3.
    But it will add some invalid properties such as `AWS_TOKEN=null`
seawinde pushed a commit to seawinde/doris that referenced this pull request Jun 5, 2024
## Proposed changes
Introduced from apache#35515

1. Fix invalid `to_int()` method logic
2. Remove unnecessary properties when creating s3 resource
Before, after recreating s3 resource, there will be some extra
properties being added to the resource properties,
such as AWS_ACCESS_KEY, but this keys are only for s3 client on BE side,
don' t needed when ping s3.
    But it will add some invalid properties such as `AWS_TOKEN=null`
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. dev/2.1.4-merged dev/3.0.0-merged reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants