Skip to content

[fix](s3filewriter) Fix s3_write_buffer_size boundary issue #47333

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
Merged
gavinchou merged 1 commit into from
Jan 23, 2025
Merged

[fix](s3filewriter) Fix s3_write_buffer_size boundary issue #47333

gavinchou merged 1 commit into from
Jan 23, 2025

Conversation

@gavinchou
Copy link
Contributor

@gavinchou gavinchou commented Jan 22, 2025

What problem does this PR solve?

introduced by #43254

Problem Summary:

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

@Thearas
Copy link
Contributor

Thearas commented Jan 22, 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?

@gavinchou gavinchou changed the title (s3filewriter) Fix s3_write_buffer_size boundary issue [fix](s3filewriter) Fix s3_write_buffer_size boundary issue Jan 22, 2025
@gavinchou
Copy link
Contributor Author

gavinchou commented Jan 22, 2025

run buildall

@doris-robot
Copy link

doris-robot commented Jan 22, 2025

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

------ Round 1 ----------------------------------
q1	17589	5529	5366	5366
q2	2050	307	171	171
q3	10425	1222	730	730
q4	10250	967	525	525
q5	8203	2362	2156	2156
q6	198	174	135	135
q7	911	749	611	611
q8	9241	1352	1191	1191
q9	5163	4879	4875	4875
q10	6856	2344	1870	1870
q11	483	268	252	252
q12	345	354	229	229
q13	17979	3824	3231	3231
q14	237	230	206	206
q15	538	479	474	474
q16	630	619	595	595
q17	607	880	347	347
q18	6902	6470	6466	6466
q19	1822	954	542	542
q20	303	321	185	185
q21	2875	2130	1966	1966
q22	364	338	308	308
Total cold run time: 103971 ms
Total hot run time: 32431 ms

----- Round 2, with runtime_filter_mode=off -----
q1	5543	5465	5520	5465
q2	243	326	236	236
q3	2242	2633	2325	2325
q4	1380	1800	1355	1355
q5	4283	4712	4620	4620
q6	172	160	129	129
q7	2107	1948	1845	1845
q8	2613	2823	2693	2693
q9	7339	7220	7295	7220
q10	3026	3293	2825	2825
q11	566	505	478	478
q12	650	708	568	568
q13	3510	3925	3304	3304
q14	299	310	273	273
q15	516	474	465	465
q16	653	700	655	655
q17	1218	1731	1255	1255
q18	7775	7402	7472	7402
q19	815	1169	1044	1044
q20	2029	2072	1905	1905
q21	5796	5435	4997	4997
q22	635	632	577	577
Total cold run time: 53410 ms
Total hot run time: 51636 ms

@doris-robot
Copy link

doris-robot commented Jan 22, 2025

TeamCity be ut coverage result:
Function Coverage: 41.56% (10836/26071)
Line Coverage: 31.97% (91616/286604)
Region Coverage: 31.09% (46915/150880)
Branch Coverage: 27.19% (23763/87392)
Coverage Report: http://coverage.selectdb-in.cc/coverage/ac06fcfebe1089ecd5aae011d2116db0c7110e8c_ac06fcfebe1089ecd5aae011d2116db0c7110e8c/report/index.html

@doris-robot
Copy link

doris-robot commented Jan 22, 2025

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

query1	1349	982	928	928
query2	6412	2147	2105	2105
query3	10959	4396	4332	4332
query4	60550	29116	22945	22945
query5	5518	586	469	469
query6	439	189	205	189
query7	5595	494	308	308
query8	326	238	234	234
query9	8415	2690	2666	2666
query10	456	298	240	240
query11	17610	15223	15435	15223
query12	154	115	108	108
query13	1414	554	429	429
query14	11509	6739	7202	6739
query15	206	206	202	202
query16	7168	595	478	478
query17	1108	780	571	571
query18	1739	431	299	299
query19	219	188	152	152
query20	123	113	108	108
query21	211	123	135	123
query22	4592	4840	4494	4494
query23	33869	33951	33181	33181
query24	5549	2301	2302	2301
query25	483	494	396	396
query26	649	239	155	155
query27	1744	480	346	346
query28	4300	2517	2476	2476
query29	574	556	439	439
query30	225	190	160	160
query31	920	900	855	855
query32	71	73	63	63
query33	425	365	326	326
query34	734	875	515	515
query35	809	876	784	784
query36	1041	1039	956	956
query37	139	110	79	79
query38	4347	4360	4223	4223
query39	1506	1435	1430	1430
query40	208	117	99	99
query41	51	46	49	46
query42	119	100	101	100
query43	533	540	491	491
query44	1325	821	817	817
query45	195	175	167	167
query46	869	1076	645	645
query47	1940	1972	1878	1878
query48	393	438	323	323
query49	699	493	396	396
query50	651	679	390	390
query51	7019	7107	6871	6871
query52	107	99	94	94
query53	229	253	191	191
query54	473	525	457	457
query55	84	79	84	79
query56	259	258	261	258
query57	1176	1181	1117	1117
query58	234	225	229	225
query59	3208	3225	3008	3008
query60	294	265	239	239
query61	119	118	121	118
query62	699	736	671	671
query63	220	186	179	179
query64	1239	1027	693	693
query65	3230	3146	3210	3146
query66	728	412	291	291
query67	15779	15755	15552	15552
query68	5048	844	557	557
query69	484	301	269	269
query70	1182	1176	1141	1141
query71	424	287	253	253
query72	6032	3866	3854	3854
query73	809	745	371	371
query74	9816	8975	9217	8975
query75	3237	3109	2671	2671
query76	3752	1169	754	754
query77	493	360	266	266
query78	10270	9998	9305	9305
query79	3331	805	644	644
query80	1580	543	476	476
query81	556	280	233	233
query82	477	153	128	128
query83	267	172	150	150
query84	286	103	70	70
query85	786	350	299	299
query86	465	324	302	302
query87	4408	4425	4406	4406
query88	4652	2190	2182	2182
query89	410	339	288	288
query90	1560	190	199	190
query91	134	142	109	109
query92	62	57	53	53
query93	2874	876	530	530
query94	777	401	286	286
query95	329	266	256	256
query96	484	611	281	281
query97	2826	2859	2778	2778
query98	226	200	246	200
query99	1294	1369	1233	1233
Total cold run time: 314868 ms
Total hot run time: 194078 ms

@doris-robot
Copy link

doris-robot commented Jan 22, 2025

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

query1	0.04	0.04	0.03
query2	0.07	0.03	0.03
query3	0.23	0.07	0.06
query4	1.62	0.10	0.10
query5	0.41	0.41	0.41
query6	1.14	0.66	0.65
query7	0.02	0.01	0.01
query8	0.04	0.03	0.03
query9	0.60	0.50	0.51
query10	0.54	0.56	0.54
query11	0.15	0.10	0.10
query12	0.14	0.11	0.11
query13	0.62	0.63	0.59
query14	2.84	2.72	2.75
query15	0.90	0.82	0.82
query16	0.38	0.40	0.39
query17	1.02	1.00	1.00
query18	0.23	0.21	0.21
query19	1.83	1.83	2.00
query20	0.01	0.01	0.01
query21	15.36	0.94	0.58
query22	0.77	0.84	0.77
query23	15.09	1.39	0.64
query24	3.25	1.19	1.56
query25	0.11	0.29	0.05
query26	0.38	0.14	0.13
query27	0.04	0.04	0.05
query28	14.39	0.99	0.43
query29	12.59	3.98	3.33
query30	0.25	0.09	0.06
query31	2.83	0.61	0.38
query32	3.22	0.55	0.46
query33	2.98	3.01	3.04
query34	16.51	5.31	4.46
query35	4.62	4.54	4.50
query36	0.65	0.48	0.47
query37	0.09	0.06	0.06
query38	0.04	0.04	0.03
query39	0.03	0.02	0.02
query40	0.17	0.15	0.12
query41	0.07	0.02	0.02
query42	0.03	0.02	0.03
query43	0.04	0.03	0.02
Total cold run time: 106.34 s
Total hot run time: 30.93 s

swjtu-zhanglei
swjtu-zhanglei reviewed Jan 23, 2025
View reviewed changes
be/src/io/fs/s3_file_writer.cpp
}

if (_failed || _completed_parts.size() != _cur_part_num) {
// check number of parts
Copy link
Contributor

@swjtu-zhanglei swjtu-zhanglei Jan 23, 2025

Choose a reason for hiding this comment

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

why not just use a flag to distinguish put_object and multipart putobj

dataroaring
dataroaring approved these changes Jan 23, 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

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Jan 23, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jan 23, 2025

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

@github-actions
Copy link
Contributor

github-actions bot commented Jan 23, 2025

PR approved by anyone and no changes requested.

@gavinchou gavinchou merged commit 1cc3d25 into apache:master Jan 23, 2025
28 of 32 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 23, 2025
@gavinchou
[fix](s3filewriter) Fix s3_write_buffer_size boundary issue (#47333)
bebc75b 
introduced by #43254
@github-actions github-actions bot mentioned this pull request Jan 23, 2025
Merged
gavinchou added a commit that referenced this pull request Jan 23, 2025
@github-actions @gavinchou
branch-3.0: [fix](s3filewriter) Fix s3_write_buffer_size boundary issue
ae129e7 
#47333 (#47341)

Cherry-picked from #47333

Co-authored-by: Gavin Chou <gavin@selectdb.com>
BiteTheDDDDt pushed a commit to BiteTheDDDDt/incubator-doris that referenced this pull request Feb 7, 2025
@gavinchou
[fix](s3filewriter) Fix s3_write_buffer_size boundary issue apache#47333
e6696c5
@gavinchou gavinchou mentioned this pull request Feb 18, 2025
Closed
lzyy2024 pushed a commit to lzyy2024/doris that referenced this pull request Feb 21, 2025
@gavinchou @lzyy2024
[fix](s3filewriter) Fix s3_write_buffer_size boundary issue (apache#4…
bc87df9 
…7333)

introduced by apache#43254
koarz pushed a commit to koarz/doris that referenced this pull request Jun 4, 2025
@gavinchou @koarz
[fix](s3filewriter) Fix s3_write_buffer_size boundary issue (apache#4…
27f98df 
…7333)

introduced by apache#43254
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dataroaring dataroaring dataroaring approved these changes

+1 more reviewer

@swjtu-zhanglei swjtu-zhanglei swjtu-zhanglei approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by one committer. dev/3.0.4-merged p0_c reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gavinchou @Thearas @doris-robot @swjtu-zhanglei @dataroaring