Skip to content

Conversation

@zy-kkk
Copy link
Member

@zy-kkk zy-kkk commented Mar 14, 2024

Proposed changes

Issue Number: close #xxx

Call to AttachCurrentThread failed with error: -1
getJNIEnv: getGlobalJNIEnv failed
*** Query id: fcd93e45c5df4271-95179a8e8523a9df ***
*** is nereids: 1 ***
*** tablet id: 0 ***
*** Aborted at 1710361477 (unix time) try "date -d @1710361477" if you are using GNU date ***
*** Current BE git commitID: 2f4401189a ***
*** SIGSEGV address not mapped to object (@0x0) received by PID 90049 (TID 97639 OR 0x7f83b1cb9700) from PID 0; stack trace: ***
 0# doris::signal::(anonymous namespace)::FailureSignalHandler(int, siginfo_t*, void*) at /home/zcp/repo_center/doris_branch-2.1/doris/be/src/common/signal_handler.h:421
 1# 0x00007F8BCF8770A7 in /usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server/libjvm.so
 2# JVM_handle_linux_signal in /usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server/libjvm.so
 3# 0x00007F8BCF87002C in /usr/lib/jvm/java-8-openjdk-amd64/jre/lib/amd64/server/libjvm.so
 4# 0x00007F8BD7C38090 in /lib/x86_64-linux-gnu/libc.so.6
 5# JNIEnv_::DeleteGlobalRef(_jobject*) at /usr/lib/jvm/java-8-openjdk-amd64/include/jni.h:848
 6# doris::vectorized::JdbcConnector::close(doris::Status) at /home/zcp/repo_center/doris_branch-2.1/doris/be/src/vec/exec/vjdbc_connector.cpp:88
 7# doris::vectorized::NewJdbcScanner::close(doris::RuntimeState*) at /home/zcp/repo_center/doris_branch-2.1/doris/be/src/vec/exec/scan/new_jdbc_scanner.cpp:207

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@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

@zy-kkk
Copy link
Member Author

zy-kkk commented Mar 14, 2024

run buildall

@github-actions
Copy link
Contributor

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

morningman
morningman previously approved these changes Mar 14, 2024
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
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 Mar 14, 2024
@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 35.01% (8576/24499)
Line Coverage: 26.76% (69478/259626)
Region Coverage: 26.03% (36082/138611)
Branch Coverage: 22.98% (18421/80152)
Coverage Report: http://coverage.selectdb-in.cc/coverage/de2c0740ad81d982803e450693a47f569b0c306e_de2c0740ad81d982803e450693a47f569b0c306e/report/index.html

JNIEnv* env;
RETURN_IF_ERROR(JniUtil::GetJNIEnv(&env));
JNIEnv* env = nullptr;
Status status = JniUtil::GetJNIEnv(&env);
Copy link
Contributor

Choose a reason for hiding this comment

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

There are many APIs that call GetJNIEnv, and use RETURN_IF_ERROR(JniUtil::GetJNIEnv(&env));
I think we should add check in GetJNIEnv if the result == nullptr, should return an error status.

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17663	4117	4106	4106
q2	2020	154	148	148
q3	10594	1066	886	886
q4	7435	703	714	703
q5	7463	2626	2625	2625
q6	184	123	123	123
q7	1218	834	808	808
q8	9335	1992	1994	1992
q9	7227	6414	6376	6376
q10	8512	3502	3569	3502
q11	432	228	223	223
q12	795	312	315	312
q13	18008	2891	2884	2884
q14	283	248	247	247
q15	494	458	448	448
q16	500	388	388	388
q17	936	573	554	554
q18	7132	6440	6472	6440
q19	1542	1410	1445	1410
q20	544	293	281	281
q21	6172	3619	3669	3619
q22	352	308	298	298
Total cold run time: 108841 ms
Total hot run time: 38373 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4101	4120	4096	4096
q2	321	233	222	222
q3	2939	2837	2809	2809
q4	1811	1506	1542	1506
q5	5171	5268	5243	5243
q6	192	118	124	118
q7	2231	1855	1865	1855
q8	3160	3268	3270	3268
q9	8526	8579	8565	8565
q10	3676	3663	3634	3634
q11	546	439	447	439
q12	712	571	547	547
q13	16926	2858	2843	2843
q14	270	247	255	247
q15	487	448	451	448
q16	452	416	416	416
q17	1715	1481	1464	1464
q18	7431	7173	7125	7125
q19	3669	1491	1509	1491
q20	1907	1742	1694	1694
q21	4969	4854	4791	4791
q22	496	451	439	439
Total cold run time: 71708 ms
Total hot run time: 53260 ms

@github-actions github-actions bot removed the approved Indicates a PR has been approved by one committer. label Mar 14, 2024
morningman
morningman previously approved these changes Mar 14, 2024
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 bot added the approved Indicates a PR has been approved by one committer. label Mar 14, 2024
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

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

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

@yiguolei
Copy link
Contributor

run buildall

@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 Mar 14, 2024
@doris-robot
Copy link

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

------ Round 1 ----------------------------------
q1	17657	4238	4174	4174
q2	2036	156	150	150
q3	10596	1109	893	893
q4	7770	785	690	690
q5	7469	2654	2659	2654
q6	183	128	125	125
q7	1239	844	840	840
q8	9386	1979	2003	1979
q9	7169	6459	6476	6459
q10	8484	3538	3670	3538
q11	421	240	232	232
q12	616	309	291	291
q13	17761	2845	2868	2845
q14	292	246	245	245
q15	493	462	460	460
q16	489	396	394	394
q17	944	573	578	573
q18	7434	6549	6457	6457
q19	3374	1502	1405	1405
q20	573	291	287	287
q21	6252	3563	3586	3563
q22	370	308	313	308
Total cold run time: 111008 ms
Total hot run time: 38562 ms

----- Round 2, with runtime_filter_mode=off -----
q1	4166	4108	4089	4089
q2	320	227	221	221
q3	3008	2843	2854	2843
q4	1895	1574	1523	1523
q5	5235	5238	5272	5238
q6	197	115	115	115
q7	2268	1865	1850	1850
q8	3141	3268	3279	3268
q9	8601	8586	8596	8586
q10	3668	3695	3720	3695
q11	553	441	444	441
q12	724	552	547	547
q13	16919	2857	2820	2820
q14	270	245	268	245
q15	488	440	450	440
q16	455	417	415	415
q17	1718	1500	1457	1457
q18	7367	7144	7128	7128
q19	1601	1548	1558	1548
q20	1930	1709	1709	1709
q21	4818	4784	4778	4778
q22	533	431	450	431
Total cold run time: 69875 ms
Total hot run time: 53387 ms

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 35.00% (8573/24493)
Line Coverage: 26.75% (69457/259670)
Region Coverage: 26.02% (36077/138631)
Branch Coverage: 22.97% (18415/80166)
Coverage Report: http://coverage.selectdb-in.cc/coverage/aa9b835f6fc452b0a2277cf26853bf2265c514fc_aa9b835f6fc452b0a2277cf26853bf2265c514fc/report/index.html

@wm1581066 wm1581066 added the p0_c label Mar 14, 2024
@yiguolei yiguolei merged commit 60f97ab into apache:master Mar 14, 2024
@zy-kkk zy-kkk deleted the fix_jdbc_core branch March 14, 2024 09:41
zy-kkk added a commit to zy-kkk/doris that referenced this pull request Mar 15, 2024
morningman added a commit that referenced this pull request Mar 29, 2024
This PR #32217 find a problem that may failed to get jni env.
And it did a work around to avoid BE crash.

This PR followup this issue, to avoid BE crash when doing `close()` of JniConnector
if failed to get jni env.

The `close()` method will return error when:
1. Failed to get jni env
2. Failed to release jni resource.

This PR will ignore the first error, and still log fatal for second error
morningman added a commit to morningman/doris that referenced this pull request Mar 29, 2024
This PR apache#32217 find a problem that may failed to get jni env.
And it did a work around to avoid BE crash.

This PR followup this issue, to avoid BE crash when doing `close()` of JniConnector
if failed to get jni env.

The `close()` method will return error when:
1. Failed to get jni env
2. Failed to release jni resource.

This PR will ignore the first error, and still log fatal for second error
yiguolei pushed a commit that referenced this pull request Apr 1, 2024
This PR #32217 find a problem that may failed to get jni env.
And it did a work around to avoid BE crash.

This PR followup this issue, to avoid BE crash when doing `close()` of JniConnector
if failed to get jni env.

The `close()` method will return error when:
1. Failed to get jni env
2. Failed to release jni resource.

This PR will ignore the first error, and still log fatal for second error
morningman added a commit to morningman/doris that referenced this pull request Apr 7, 2024
This PR apache#32217 find a problem that may failed to get jni env.
And it did a work around to avoid BE crash.

This PR followup this issue, to avoid BE crash when doing `close()` of JniConnector
if failed to get jni env.

The `close()` method will return error when:
1. Failed to get jni env
2. Failed to release jni resource.

This PR will ignore the first error, and still log fatal for second error
mongo360 pushed a commit to mongo360/doris that referenced this pull request Aug 16, 2024
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.0.7-merged p0_c reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants