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

[KYUUBI #6843] Fix 'query-timeout-thread' thread leak #6844

Closed
wants to merge 5 commits into from

Conversation

ASiegeLion
Copy link
Contributor

@ASiegeLion ASiegeLion commented Dec 9, 2024

Why are the changes needed?

see #6843

If the session manager's ThreadPoolExecutor refuses to execute asyncOperation, then we need to shut down the query-timeout-thread in the catch

How was this patch tested?

1 Use jstack to view threads on the long-lived engine side
image

2 Wait for all SQL statements in the engine to finish executing, and then use stack to check the number of query-timeout-thread threads, which should be empty.
image

Was this patch authored or co-authored using generative AI tooling?

NO

@wForget
Copy link
Member

wForget commented Dec 12, 2024

Please fix the failed checks.

@pan3793
Copy link
Member

pan3793 commented Dec 16, 2024

@ASiegeLion please write the PR description seriously, and revert unnecessary code changes.

@ASiegeLion
Copy link
Contributor Author

@ASiegeLion please write the PR description seriously, and revert unnecessary code changes.

ok

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (dc3ac89) to head (9107a30).
Report is 21 commits behind head on master.

Files with missing lines Patch % Lines
.../kyuubi/engine/spark/operation/ExecutePython.scala 0.00% 1 Missing ⚠️
...e/kyuubi/engine/spark/operation/ExecuteScala.scala 0.00% 1 Missing ⚠️
...uubi/engine/spark/operation/ExecuteStatement.scala 0.00% 1 Missing ⚠️
.../org/apache/kyuubi/operation/KyuubiOperation.scala 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #6844   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         687     687           
  Lines       42463   42467    +4     
  Branches     5796    5796           
======================================
- Misses      42463   42467    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cxzl25
Copy link
Contributor

cxzl25 commented Dec 16, 2024

We also fixed this issue a few months ago.

cc @lsm1

@pan3793 pan3793 changed the title [KYUUBI #6843] FIX 'query-timeout-thread' thread leak [KYUUBI #6843] Fix 'query-timeout-thread' thread leak Dec 16, 2024
@pan3793 pan3793 requested a review from lsm1 December 24, 2024 03:36
@lsm1
Copy link
Contributor

lsm1 commented Dec 24, 2024

@ASiegeLion In other engines, there might be the same issue. Please fix it.

@pan3793 pan3793 added this to the v1.9.4 milestone Dec 27, 2024
@pan3793 pan3793 closed this in a051253 Dec 27, 2024
pan3793 pushed a commit that referenced this pull request Dec 27, 2024
### Why are the changes needed?

see #6843

If the session manager's ThreadPoolExecutor refuses to execute asyncOperation,   then we need to shut down the query-timeout-thread in the catch

### How was this patch tested?

 1 Use jstack to view threads on the long-lived engine side
![image](https://github.com/user-attachments/assets/95d3a897-001d-4250-bf13-172b6997021b)

 2  Wait for all SQL statements in the engine to finish executing, and then use stack to check the number of query-timeout-thread threads, which should be empty.
![image](https://github.com/user-attachments/assets/0afbc026-7dd3-4594-afd2-92a5ef23f6cb)

### Was this patch authored or co-authored using generative AI tooling?

NO

Closes #6844 from ASiegeLion/master.

Closes #6843

9107a30 [liupeiyue] [KYUUBI #6843] FIX 'query-timeout-thread' thread leak
4b3417f [liupeiyue] [KYUUBI #6843] FIX 'query-timeout-thread' thread leak
ef1f66b [liupeiyue] [KYUUBI #6843] FIX 'query-timeout-thread' thread leak
9e1a015 [liupeiyue] [KYUUBI #6843] FIX 'query-timeout-thread' thread leak
78a9fde [liupeiyue] [KYUUBI #6843] FIX 'query-timeout-thread' thread leak

Authored-by: liupeiyue <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
(cherry picked from commit a051253)
Signed-off-by: Cheng Pan <[email protected]>
pan3793 pushed a commit that referenced this pull request Dec 27, 2024
### Why are the changes needed?

see #6843

If the session manager's ThreadPoolExecutor refuses to execute asyncOperation,   then we need to shut down the query-timeout-thread in the catch

### How was this patch tested?

 1 Use jstack to view threads on the long-lived engine side
![image](https://github.com/user-attachments/assets/95d3a897-001d-4250-bf13-172b6997021b)

 2  Wait for all SQL statements in the engine to finish executing, and then use stack to check the number of query-timeout-thread threads, which should be empty.
![image](https://github.com/user-attachments/assets/0afbc026-7dd3-4594-afd2-92a5ef23f6cb)

### Was this patch authored or co-authored using generative AI tooling?

NO

Closes #6844 from ASiegeLion/master.

Closes #6843

9107a30 [liupeiyue] [KYUUBI #6843] FIX 'query-timeout-thread' thread leak
4b3417f [liupeiyue] [KYUUBI #6843] FIX 'query-timeout-thread' thread leak
ef1f66b [liupeiyue] [KYUUBI #6843] FIX 'query-timeout-thread' thread leak
9e1a015 [liupeiyue] [KYUUBI #6843] FIX 'query-timeout-thread' thread leak
78a9fde [liupeiyue] [KYUUBI #6843] FIX 'query-timeout-thread' thread leak

Authored-by: liupeiyue <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
(cherry picked from commit a051253)
Signed-off-by: Cheng Pan <[email protected]>
@pan3793
Copy link
Member

pan3793 commented Dec 27, 2024

Seems the contributor is slient a while, let me merge this, and @lsm1 could you please send a followup to cover the rest?

@pan3793
Copy link
Member

pan3793 commented Dec 27, 2024

Merged to 1.11.0/1.10.2/1.9.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants