Skip to content

Only force close engine ref when open session failed #4241

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

Closed
ulysses-you wants to merge 2 commits into from
Closed

Only force close engine ref when open session failed #4241

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1636912
Only force close engine ref when open session failed
ulysses-you Feb 3, 2023
0b28041
style
ulysses-you Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion kyuubi-server/src/main/scala/org/apache/kyuubi/session/KyuubiSessionImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ class KyuubiSessionImpl(

private var _engineSessionHandle: SessionHandle = _

private var openSessionError: Option[Throwable] = None

override def open(): Unit = handleSessionException {
traceMetricsOnOpen()

Expand Down Expand Up @@ -170,6 +172,7 @@ class KyuubiSessionImpl(
s"Opening engine [${engine.defaultEngineName} $host:$port]" +
s" for $user session failed",
e)
openSessionError = Some(e)
throw e
} finally {
attempt += 1
Expand Down Expand Up @@ -247,7 +250,7 @@ class KyuubiSessionImpl(
try {
if (_client != null) _client.closeSession()
} finally {
if (engine != null) engine.close()
openSessionError.foreach { _ => if (engine != null) engine.close() }
Copy link
Contributor

@yikf yikf Feb 3, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@ulysses-you ulysses-you Feb 6, 2023

Choose a reason for hiding this comment

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

With connection share level, engine should close gracefully by itself. Engine exit code will not be 0 if we close engine by Kyuubi Server.

Copy link
Member

@wForget wForget Feb 6, 2023

Choose a reason for hiding this comment

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

Can we add a timeout to wait for the engine status to complete, and then kill the application.

Copy link
Contributor Author

@ulysses-you ulysses-you Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@wForget wForget Feb 6, 2023

Choose a reason for hiding this comment

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

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.

I agree with you, this pr is enough to fix the previous issue.

sessionEvent.endTime = System.currentTimeMillis()
EventBus.post(sessionEvent)
traceMetricsOnClose()
Expand Down