[KYUUBI #2503][FOLLOWUP] Catch all exceptions for sessions timeout check task #5727

turboFei · 2023-11-19T04:04:19Z

🔍 Description

#2503 followup

log some message when executing timeout check task
catch all exceptions when checking sessions timeout
BTW: for logSessionCountInfo, log the session class simple name to provide more insights, such as KyuubiSessionImpl or KyuubiBatchSession.

Issue References 🔗

This pull request fixes #

Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Types of changes 🔖

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklists

📝 Author Self Checklist

My code follows the style guidelines of this project
I have performed a self-review
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
This patch was not authored or co-authored using Generative Tooling

📝 Committer Pre-Merge Checklist

Be nice. Be informative.

log session type

codecov-commenter · 2023-11-19T05:17:36Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (3478fc9) 61.35% compared to head (d726a84) 61.29%.

Files	Patch %	Lines
...ala/org/apache/kyuubi/session/SessionManager.scala	88.88%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5727      +/-   ##
============================================
- Coverage     61.35%   61.29%   -0.07%     
  Complexity       23       23              
============================================
  Files           607      607              
  Lines         35820    35821       +1     
  Branches       4914     4914              
============================================
- Hits          21979    21958      -21     
- Misses        11459    11470      +11     
- Partials       2382     2393      +11

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

wForget

LGTM

…eck task # 🔍 Description #2503 followup 1. log some message when executing timeout check task 2. catch all exceptions when checking sessions timeout BTW: for `logSessionCountInfo`, log the session class simple name to provide more insights, such as `KyuubiSessionImpl` or `KyuubiBatchSession`. ## Issue References 🔗 This pull request fixes # ## Describe Your Solution 🔧 Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklists ## 📝 Author Self Checklist - [ ] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [ ] I have performed a self-review - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [ ] Pull request title is okay. - [ ] No license issues. - [ ] Milestone correctly set? - [ ] Test coverage is ok - [ ] Assignees are selected. - [ ] Minimum number of approvals - [ ] No changes are requested **Be nice. Be informative.** Closes #5727 from turboFei/catch_all_exception. Closes #2503 d726a84 [fwang12] catch all Authored-by: fwang12 <fwang12@ebay.com> Signed-off-by: fwang12 <fwang12@ebay.com> (cherry picked from commit 37641f3) Signed-off-by: fwang12 <fwang12@ebay.com>

turboFei · 2023-11-20T03:01:42Z

thanks, merged to master and branch-1.8

# 🔍 Description ## Issue References 🔗 As the PR #5727 fixed a problem caused by leaking exception in a scheduled task, the exception thrown should be properly handled to prevent suspension in `ScheduledExecutorService.scheduleWithFixedDelay`. ## Describe Your Solution 🔧 Introducing a util method `scheduleTolerableRunnableWithFixedDelay` for catching possible exceptions in scheduled tasks. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ No behaviour change. #### Behavior With This Pull Request 🎉 This is a fallback measure to ensure all the exceptions handled. Currently all the occurrences are properly handled. #### Related Unit Tests Pass all the CI tests. --- # Checklists ## 📝 Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [ ] Pull request title is okay. - [ ] No license issues. - [ ] Milestone correctly set? - [ ] Test coverage is ok - [ ] Assignees are selected. - [ ] Minimum number of approvals - [ ] No changes are requested **Be nice. Be informative.** Closes #5730 from bowenliang123/tolerate-runnable. Closes #5730 a38eda0 [Bowen Liang] use thread name for error message 6bc13b7 [Bowen Liang] comment 87e2bf2 [Bowen Liang] schedule tolerable runnable Authored-by: Bowen Liang <liangbowen@gf.com.cn> Signed-off-by: fwang12 <fwang12@ebay.com>

# 🔍 Description ## Issue References 🔗 As the PR #5727 fixed a problem caused by leaking exception in a scheduled task, the exception thrown should be properly handled to prevent suspension in `ScheduledExecutorService.scheduleWithFixedDelay`. ## Describe Your Solution 🔧 Introducing a util method `scheduleTolerableRunnableWithFixedDelay` for catching possible exceptions in scheduled tasks. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ No behaviour change. #### Behavior With This Pull Request 🎉 This is a fallback measure to ensure all the exceptions handled. Currently all the occurrences are properly handled. #### Related Unit Tests Pass all the CI tests. --- # Checklists ## 📝 Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [ ] Pull request title is okay. - [ ] No license issues. - [ ] Milestone correctly set? - [ ] Test coverage is ok - [ ] Assignees are selected. - [ ] Minimum number of approvals - [ ] No changes are requested **Be nice. Be informative.** Closes #5730 from bowenliang123/tolerate-runnable. Closes #5730 a38eda0 [Bowen Liang] use thread name for error message 6bc13b7 [Bowen Liang] comment 87e2bf2 [Bowen Liang] schedule tolerable runnable Authored-by: Bowen Liang <liangbowen@gf.com.cn> Signed-off-by: fwang12 <fwang12@ebay.com> (cherry picked from commit 8480605) Signed-off-by: fwang12 <fwang12@ebay.com>

…out check task # 🔍 Description apache#2503 followup 1. log some message when executing timeout check task 2. catch all exceptions when checking sessions timeout BTW: for `logSessionCountInfo`, log the session class simple name to provide more insights, such as `KyuubiSessionImpl` or `KyuubiBatchSession`. ## Issue References 🔗 This pull request fixes # ## Describe Your Solution 🔧 Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change. ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklists ## 📝 Author Self Checklist - [ ] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [ ] I have performed a self-review - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [ ] Pull request title is okay. - [ ] No license issues. - [ ] Milestone correctly set? - [ ] Test coverage is ok - [ ] Assignees are selected. - [ ] Minimum number of approvals - [ ] No changes are requested **Be nice. Be informative.** Closes apache#5727 from turboFei/catch_all_exception. Closes apache#2503 d726a84 [fwang12] catch all Authored-by: fwang12 <fwang12@ebay.com> Signed-off-by: fwang12 <fwang12@ebay.com> (cherry picked from commit 37641f3) Signed-off-by: fwang12 <fwang12@ebay.com>

catch all

d726a84

log session type

github-actions bot added the module:common label Nov 19, 2023

turboFei requested a review from wForget November 19, 2023 04:07

turboFei changed the title ~~Catch all exceptions for sessions timeout check task~~ [KYUUBI #2503][FOLLOWUP] Catch all exceptions for sessions timeout check task Nov 19, 2023

turboFei self-assigned this Nov 19, 2023

turboFei requested a review from pan3793 November 19, 2023 07:25

wForget approved these changes Nov 20, 2023

View reviewed changes

turboFei closed this in 37641f3 Nov 20, 2023

turboFei added this to the v1.8.1 milestone Nov 20, 2023

turboFei deleted the catch_all_exception branch November 20, 2023 03:01

bowenliang123 mentioned this pull request Nov 20, 2023

Tolerate exceptions for periodical scheduled tasks #5730

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[KYUUBI #2503][FOLLOWUP] Catch all exceptions for sessions timeout check task #5727

[KYUUBI #2503][FOLLOWUP] Catch all exceptions for sessions timeout check task #5727

Uh oh!

turboFei commented Nov 19, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented Nov 19, 2023

Uh oh!

wForget left a comment

Uh oh!

turboFei commented Nov 20, 2023

Uh oh!

Uh oh!

[KYUUBI #2503][FOLLOWUP] Catch all exceptions for sessions timeout check task #5727

[KYUUBI #2503][FOLLOWUP] Catch all exceptions for sessions timeout check task #5727

Uh oh!

Conversation

turboFei commented Nov 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Description

Issue References 🔗

Describe Your Solution 🔧

Types of changes 🔖

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklists

📝 Author Self Checklist

📝 Committer Pre-Merge Checklist

Uh oh!

codecov-commenter commented Nov 19, 2023

Codecov Report

Uh oh!

wForget left a comment

Choose a reason for hiding this comment

Uh oh!

turboFei commented Nov 20, 2023

Uh oh!

Uh oh!

turboFei commented Nov 19, 2023 •

edited

Loading