Only force close engine ref when open session failed #4241
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4241 +/- ##
============================================
- Coverage 53.59% 53.48% -0.11%
Complexity 13 13
============================================
Files 560 560
Lines 30422 30420 -2
Branches 4084 4081 -3
============================================
- Hits 16304 16270 -34
- Misses 12614 12645 +31
- Partials 1504 1505 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| if (_client != null) _client.closeSession() | ||
| } finally { | ||
| if (engine != null) engine.close() | ||
| openSessionError.foreach { _ => if (engine != null) engine.close() } |
There was a problem hiding this comment.
The egine will not be reused if the sharelevel is a CONNECTION and the session is closed, so should close engine whether open session fails or not?
There was a problem hiding this comment.
With connection share level, engine should close gracefully by itself. Engine exit code will not be 0 if we close engine by Kyuubi Server.
There was a problem hiding this comment.
Can we add a timeout to wait for the engine status to complete, and then kill the application.
There was a problem hiding this comment.
We can, but it's a bit complex. I think this pr is enough to fix regression.
I see your requreiements that to avoid the leaked engine. We can improve that in a global view rather than wait and kill one by one. e.g. Use a thread to detect if there is a leaked engine.
Note, it's the key to define what is the leaked engine. In general, A leaked engine can be:
- status is finished in resource manager but engine build process is alive (for all share level)
- session is closed but status is not fiinshed or engine build process is alive (for connection share level and batch mode)
We can have a new pr if you are interested in.
There was a problem hiding this comment.
We can, but it's a bit complex. I think this pr is enough to fix regression.
I see your requreiements that to avoid the leaked engine. We can improve that in a global view rather than wait and kill one by one. e.g. Use a thread to detect if there is a leaked engine.
Note, it's the key to define what is the
leaked engine. In general, Aleaked enginecan be:
- status is finished in resource manager but engine build process is alive (for all share level)
- session is closed but status is not fiinshed or engine build process is alive (for connection share level and batch mode)
We can have a new pr if you are interested in.
I agree with you, this pr is enough to fix the previous issue.
|
thanks for review, merging to master/branch-1.6 |
We do not need force close engine builder and application if open session succeded. The engine itself will shutdown in connection level. This pr tries to fix regression of #2482 - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [ ] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request Closes #4241 from ulysses-you/skip-close. Closes #4241 0b28041 [ulysses-you] style 1636912 [ulysses-you] Only force close engine ref when open session failed Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: ulyssesyou <ulyssesyou@apache.org> (cherry picked from commit 8760eee) Signed-off-by: ulyssesyou <ulyssesyou@apache.org>
5 participants
Why are the changes needed?
We do not need force close engine builder and application if open session succeded. The engine itself will shutdown in connection level.
This pr tries to fix regression of #2482
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request