[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
Open
dev-donghwan wants to merge 4 commits into
dev-donghwan wants to merge 4 commits into
Conversation
…ng interpreter process is stopped
…n hook does not unregister the group id
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:InterpreterSetting.closeInterpreters()andManagedInterpreterGroup.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, becauseInterpreterGroup#equalscompares ids only, soMap.remove(key, value)would not tell the two groups apart.ProcessLauncher.stop()only destroys a process inRUNNING, so on a launch timeout the process, which never reported running, was left alive.ExecRemoteInterpreterProcess.stop()only closed the client for a process inLAUNCHED. It now destroys the process, moves the launcher toTERMINATEDand wakeswaitForReady(), sostart()fails at once instead of waiting for the timeout. A stop that comes before the process is launched makes a laterstart()fail without launching it.destroyForcibly()rather thandestroy(). With SIGTERM the interpreter's shutdown hook sendsunRegisterInterpreterProcessfor 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.ExecuteWatchdogonly callsProcess.destroy(), soProcessLauncherkeeps 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:testCloseInterpretersKeepsGroupCreatedWhileProcessStopsandtestConcurrentCloseOfLastSessionsKeepsGroupCreatedWhileProcessStopscreate a new group with the same id while the old process stops. Reverting either removal site makes its test fail.ExecRemoteInterpreterProcessTestlaunches a stand-in process that never registers.launchTimeoutDestroysTheProcessandstopWhileLaunchingEndsTheLaunchAndDestroysTheProcesscheck 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-shadedpackaged:ExecRemoteInterpreterProcessTest(2),InterpreterSettingTest(14),ManagedInterpreterGroupTest(2),InterpreterSettingManagerTest(13),RemoteInterpreterEventServerTest(5),InterpreterFactoryTest(3),RemoteInterpreterServerTest(3)RemoteInterpreterTest(17),IdleInterpreterReclaimerTest(9),RemoteInterpreterOutputTestStreamTest(4),RemoteAngularObjectTest(3)