Skip to content

[hotfix](jdbc catalog) Fix jdbcclient repeated initialization #51036

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
morningman merged 2 commits into from
May 20, 2025
Merged

[hotfix](jdbc catalog) Fix jdbcclient repeated initialization #51036

morningman merged 2 commits into from
May 20, 2025

Conversation

@zy-kkk
Copy link
Member

@zy-kkk zy-kkk commented May 19, 2025 •
edited
Loading

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #50901

Problem Summary:

In the previous PR, I added makeSureInitialized() to configureJdbcTable, which will create the jdbcclient if it does not exist. However, in the testJdbcConnection process, makeSureInitialized() is bypassed and initLocalObjectsImpl is used directly. This will cause a jdbcclient to be created again when configureJdbcTable is used in testJdbcConnection, and the previous jdbcclient is not closed, resulting in a connection leak.

In the modification of this PR, I used a completely independent testClient for the TestConnection process to avoid this problem.

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

@hello-stephen
Copy link
Contributor

hello-stephen commented May 19, 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?

@zy-kkk
Copy link
Member Author

zy-kkk commented May 19, 2025

run buildall

@zy-kkk zy-kkk force-pushed the fix_jdbcclient_init branch from 70f6c20 to ba0a00a Compare May 19, 2025 10:17
morningman
morningman approved these changes May 19, 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

@morningman
Copy link
Contributor

morningman commented May 19, 2025

run buildall

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

github-actions bot commented May 19, 2025

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

@github-actions
Copy link
Contributor

github-actions bot commented May 19, 2025

PR approved by anyone and no changes requested.

@doris-robot
Copy link

doris-robot commented May 19, 2025

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

------ Round 1 ----------------------------------
q1	26259	5030	5160	5030
q2	2073	280	189	189
q3	10666	1245	708	708
q4	10287	1006	518	518
q5	8469	2370	2292	2292
q6	185	162	130	130
q7	921	728	617	617
q8	9322	1213	1064	1064
q9	6816	5054	5090	5054
q10	6863	2321	1885	1885
q11	484	302	275	275
q12	339	341	209	209
q13	17753	3663	3117	3117
q14	221	234	211	211
q15	536	493	474	474
q16	408	446	368	368
q17	608	856	384	384
q18	7715	7158	7148	7148
q19	1550	949	551	551
q20	339	336	231	231
q21	3682	2526	2342	2342
q22	1048	1000	993	993
Total cold run time: 116544 ms
Total hot run time: 33790 ms

----- Round 2, with runtime_filter_mode=off -----
q1	5131	5049	5028	5028
q2	242	322	230	230
q3	2115	2647	2309	2309
q4	1379	1786	1346	1346
q5	4429	4409	4391	4391
q6	216	171	130	130
q7	2000	1942	1791	1791
q8	2549	2587	2505	2505
q9	7239	7217	7076	7076
q10	2996	3200	2759	2759
q11	575	519	505	505
q12	662	780	637	637
q13	3490	3861	3303	3303
q14	287	310	297	297
q15	523	477	490	477
q16	468	489	448	448
q17	1157	1563	1385	1385
q18	7700	7690	7468	7468
q19	808	814	914	814
q20	2011	2058	1871	1871
q21	4832	4573	4479	4479
q22	1108	1071	1032	1032
Total cold run time: 51917 ms
Total hot run time: 50281 ms

@doris-robot
Copy link

doris-robot commented May 19, 2025

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

query1	1411	1111	1054	1054
query2	6200	1799	1780	1780
query3	11018	4513	4314	4314
query4	53581	24756	23137	23137
query5	5077	543	452	452
query6	342	210	203	203
query7	4942	507	307	307
query8	325	263	238	238
query9	6021	2662	2671	2662
query10	459	371	303	303
query11	15544	15178	14816	14816
query12	159	113	104	104
query13	1087	534	424	424
query14	10150	6482	6311	6311
query15	219	218	183	183
query16	7069	653	510	510
query17	1091	733	597	597
query18	1577	417	324	324
query19	214	203	178	178
query20	140	127	129	127
query21	211	130	121	121
query22	4405	4480	4461	4461
query23	34413	33685	33272	33272
query24	6679	2427	2429	2427
query25	469	478	415	415
query26	705	269	163	163
query27	2301	526	360	360
query28	3143	2168	2174	2168
query29	579	556	449	449
query30	278	220	184	184
query31	874	913	772	772
query32	76	62	68	62
query33	455	380	331	331
query34	784	881	527	527
query35	819	827	767	767
query36	949	1000	874	874
query37	120	104	84	84
query38	4342	4362	4233	4233
query39	1485	1464	1479	1464
query40	216	125	108	108
query41	57	55	53	53
query42	133	114	124	114
query43	509	519	468	468
query44	1372	887	880	880
query45	190	179	168	168
query46	872	1025	651	651
query47	1833	1898	1786	1786
query48	390	462	352	352
query49	715	542	450	450
query50	692	702	418	418
query51	4215	4358	4179	4179
query52	115	112	111	111
query53	233	266	188	188
query54	599	577	521	521
query55	85	88	87	87
query56	329	332	305	305
query57	1188	1270	1137	1137
query58	268	275	261	261
query59	2690	2813	2651	2651
query60	338	330	337	330
query61	131	134	124	124
query62	740	743	688	688
query63	233	200	196	196
query64	1834	1030	690	690
query65	4347	4241	4266	4241
query66	745	400	299	299
query67	16008	15629	15308	15308
query68	7042	904	529	529
query69	543	318	279	279
query70	1220	1145	1104	1104
query71	511	321	312	312
query72	6086	4798	4942	4798
query73	1466	687	367	367
query74	9009	9166	8809	8809
query75	4022	3211	2696	2696
query76	4226	1202	764	764
query77	724	369	288	288
query78	10660	10174	9391	9391
query79	3369	825	572	572
query80	666	546	462	462
query81	467	263	212	212
query82	448	125	93	93
query83	332	264	228	228
query84	291	101	89	89
query85	820	350	310	310
query86	371	302	288	288
query87	4383	4431	4351	4351
query88	3404	2413	2408	2408
query89	404	319	290	290
query90	1934	221	207	207
query91	136	146	114	114
query92	75	62	57	57
query93	2330	942	587	587
query94	671	408	305	305
query95	375	294	281	281
query96	503	581	292	292
query97	2780	2793	2665	2665
query98	227	209	201	201
query99	1424	1377	1320	1320
Total cold run time: 301709 ms
Total hot run time: 192591 ms

@doris-robot
Copy link

doris-robot commented May 19, 2025

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

query1	0.05	0.04	0.03
query2	0.12	0.11	0.12
query3	0.25	0.20	0.20
query4	1.60	0.18	0.19
query5	0.45	0.45	0.43
query6	1.52	0.65	0.67
query7	0.02	0.01	0.02
query8	0.04	0.03	0.04
query9	0.56	0.52	0.51
query10	0.56	0.58	0.56
query11	0.15	0.11	0.11
query12	0.15	0.11	0.12
query13	0.63	0.60	0.60
query14	0.80	0.81	0.82
query15	0.88	0.85	0.87
query16	0.40	0.39	0.37
query17	1.00	1.06	1.03
query18	0.22	0.22	0.21
query19	1.94	1.88	1.90
query20	0.02	0.02	0.02
query21	15.40	0.90	0.56
query22	0.76	1.15	0.71
query23	14.90	1.40	0.62
query24	6.53	1.27	1.46
query25	0.52	0.19	0.14
query26	0.60	0.17	0.13
query27	0.06	0.05	0.06
query28	9.76	0.90	0.47
query29	12.53	4.03	3.32
query30	0.25	0.09	0.07
query31	2.81	0.61	0.38
query32	3.23	0.55	0.48
query33	3.06	3.10	3.16
query34	15.86	5.06	4.50
query35	4.54	4.52	4.50
query36	0.66	0.50	0.48
query37	0.09	0.07	0.06
query38	0.05	0.03	0.04
query39	0.04	0.03	0.02
query40	0.18	0.13	0.13
query41	0.08	0.02	0.02
query42	0.03	0.02	0.02
query43	0.04	0.04	0.03
Total cold run time: 103.34 s
Total hot run time: 29.79 s

This was referenced May 19, 2025
Merged
Merged
morningman
morningman approved these changes May 20, 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

@morningman morningman merged commit aba01d6 into apache:master May 20, 2025
27 of 28 checks passed
dataroaring pushed a commit that referenced this pull request May 20, 2025
@zy-kkk
branch-3.0: [hotfix](jdbc catalog) Fix jdbcclient repeated initializa…
ddbc99a 
…tion (#51037)

pick #51036
github-actions bot pushed a commit that referenced this pull request May 20, 2025
@zy-kkk
[hotfix](jdbc catalog) Fix jdbcclient repeated initialization (#51036)
e2649ef 
### What problem does this PR solve?
Related PR: #50901

Problem Summary:

In the previous PR, I added makeSureInitialized() to configureJdbcTable,
which will create the jdbcclient if it does not exist. However, in the
testJdbcConnection process, makeSureInitialized() is bypassed and
initLocalObjectsImpl is used directly. This will cause a jdbcclient to
be created again when configureJdbcTable is used in testJdbcConnection,
and the previous jdbcclient is not closed, resulting in a connection
leak.

In the modification of this PR, I used a completely independent
testClient for the TestConnection process to avoid this problem.
@github-actions github-actions bot mentioned this pull request May 20, 2025
Closed
@zy-kkk zy-kkk deleted the fix_jdbcclient_init branch May 20, 2025 06:20
yiguolei pushed a commit that referenced this pull request May 21, 2025
@zy-kkk
branch-2.1: [hotfix](jdbc catalog) Fix jdbcclient repeated initializa…
6fe7c90 
…tion (#51038)

pick #51036
koarz pushed a commit to koarz/doris that referenced this pull request Jun 4, 2025
@zy-kkk @koarz
[hotfix](jdbc catalog) Fix jdbcclient repeated initialization (apache…
866dd89 
…#51036)

### What problem does this PR solve?
Related PR: apache#50901

Problem Summary:

In the previous PR, I added makeSureInitialized() to configureJdbcTable,
which will create the jdbcclient if it does not exist. However, in the
testJdbcConnection process, makeSureInitialized() is bypassed and
initLocalObjectsImpl is used directly. This will cause a jdbcclient to
be created again when configureJdbcTable is used in testJdbcConnection,
and the previous jdbcclient is not closed, resulting in a connection
leak.

In the modification of this PR, I used a completely independent
testClient for the TestConnection process to avoid this problem.
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

@wuwenchi wuwenchi wuwenchi approved these changes

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by one committer. dev/2.1.11-merged dev/3.0.6-merged p0_test reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@zy-kkk @hello-stephen @morningman @doris-robot @wuwenchi @yiguolei @dataroaring