Skip to content

[ZEPPELIN-6721] Keep a new group and end the launch when an interpreter group is closed while its process is launching - #5517

Open
dev-donghwan wants to merge 4 commits into
apache:masterfrom
dev-donghwan:ZEPPELIN-6721
Open

dev-donghwan wants to merge 4 commits into
apache:masterfrom
dev-donghwan:ZEPPELIN-6721

Conversation

@dev-donghwan

Copy link
Copy Markdown
Contributor

What is this PR for?

Closing an interpreter group while a process for it is launching could leave the launch waiting for zeppelin.interpreter.connect.timeout (10 minutes by default) and the launched JVM running. This PR fixes the two paths described in the issue and cleans up the process of a launch that is given up. One commit per change:

  1. A group created while another group with the same id is closing stays registered. InterpreterSetting.closeInterpreters() and ManagedInterpreterGroup.close(sessionId) removed the group by id, so a group created with the same id while the old one was stopping was evicted, and its process's registration was then rejected. Both now remove the group only if it is still the instance registered under that id. This compares references, because InterpreterGroup#equals compares ids only, so Map.remove(key, value) would not tell the two groups apart.
  2. A launch that times out destroys its process. ProcessLauncher.stop() only destroys a process in RUNNING, so on a launch timeout the process, which never reported running, was left alive.
  3. Stopping a process that is still launching ends the launch. ExecRemoteInterpreterProcess.stop() only closed the client for a process in LAUNCHED. It now destroys the process, moves the launcher to TERMINATED and wakes waitForReady(), so start() fails at once instead of waiting for the timeout. A stop that comes before the process is launched makes a later start() fail without launching it.
  4. An abandoned launch is killed forcibly. A launch that is given up in 2 or 3 is killed with destroyForcibly() rather than destroy(). With SIGTERM the interpreter's shutdown hook sends unRegisterInterpreterProcess for its group id, and the server closes whatever group has that id, which by then can be another group (ZEPPELIN-6723). Such a process has not been initialized by the server, so it has no open interpreter to close. ExecuteWatchdog only calls Process.destroy(), so ProcessLauncher keeps the process to be able to kill it forcibly. Stopping a running process is unchanged.

The id based unregister itself is fixed separately in ZEPPELIN-6723 (#5516), since it also affects processes that are not launched by this path.

Not changed: after a launch fails, the group keeps the failed process until the interpreter is restarted, as it does today.

What type of PR is it?

Bug Fix

What is the Jira issue?

How should this be tested?

  • InterpreterSettingTest: testCloseInterpretersKeepsGroupCreatedWhileProcessStops and testConcurrentCloseOfLastSessionsKeepsGroupCreatedWhileProcessStops create a new group with the same id while the old process stops. Reverting either removal site makes its test fail.
  • ExecRemoteInterpreterProcessTest launches a stand-in process that never registers. launchTimeoutDestroysTheProcess and stopWhileLaunchingEndsTheLaunchAndDestroysTheProcess check that the launch ends and the process exits, and that it did not receive SIGTERM. Each fails without its change.

Run locally, with zeppelin-interpreter-shaded packaged:

  • ExecRemoteInterpreterProcessTest (2), InterpreterSettingTest (14), ManagedInterpreterGroupTest (2), InterpreterSettingManagerTest (13), RemoteInterpreterEventServerTest (5), InterpreterFactoryTest (3), RemoteInterpreterServerTest (3)
  • With real interpreter processes: RemoteInterpreterTest (17), IdleInterpreterReclaimerTest (9), RemoteInterpreterOutputTestStreamTest (4), RemoteAngularObjectTest (3)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant