[ZEPPELIN-6723] Close an interpreter group on unregister only when the sender is its registered process - #5516
Open
dev-donghwan wants to merge 1 commit into
Open
dev-donghwan wants to merge 1 commit into
dev-donghwan wants to merge 1 commit into
Conversation
…e sender is its registered process
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?
An interpreter process unregisters itself with only its interpreter group id, and
RemoteInterpreterEventServer.unRegisterInterpreterProcess()closes and removes whatever group currently has that id. The group with that id is not always the sender's:shutdownRPC fails while the process is alive, the process keeps its shutdown hook, is destroyed with SIGTERM a few seconds later, and the hook unregisters.In each case the new group's sessions are closed and, with the default
zeppelin.interpreter.close.cancel_job=true, its jobs are aborted.This PR sends the sender's
RegisterInfowith the unregister, records theRegisterInfothe server accepts at registration on theManagedInterpreterGroup, and closes the group only when the two match:RegisterInfo(interpreter from before this change)RegisterInfoRegisterInfoRegisterInfoRegisterInfoNotes on the choices:
unRegisterInterpreterProcess(1: string intpGroupId, 2: RegisterInfo registerInfo). The event service usesTBinaryProtocol, whose generated readers skip unknown fields, so an old server ignores the new argument and a new server receivesnullfrom an old interpreter, which keeps the id based behaviour.RegisterInfocomes from.RemoteInterpreterServerbuilds it from itshost,portandinterpreterGroupIdfields, which are set in the constructor and are what it registers with; registration and unregister now use the samegetRegisterInfo(). It is not kept inRemoteInterpreterEventClient, because the client is replaced ininit()andreconnect(). A DevInterpreter has no host and does not register, so it sendsnull.RegisterInfoit accepted, not with the process's host and port:K8sRemoteInterpreterProcess.processStarted()keepslocalhostand a forwarded port, or the pod's DNS name, instead of the registered values.InterpreterSetting.removeInterpreterGroup(ManagedInterpreterGroup)as in the ZEPPELIN-6721 fix (identical code, so either can be merged first).InterpreterSettingManager.removeInterpreterGroup(String)had no other caller and is removed.genthrift.sh. OnlyRemoteInterpreterEventService.javahas real changes; the other generated files change only in their@Generateddate, as in ZEPPELIN-6704. Before changing the IDL I regenerated master's IDL with the same compiler and got output identical to master except for those date lines.This fixes the Thrift path, which is what runs today and until ZEPPELIN-6601 makes gRPC the default (ZEPPELIN-6617, ZEPPELIN-6622). The gRPC runtime covers the same collision through its launch registry (ZEPPELIN-6607) and stale-unregister fencing (ZEPPELIN-6614); this change keeps old interpreters working and goes away with the Thrift removal in ZEPPELIN-6623. It does not overlap with the RPC contract harness in #5375.
Not covered:
isExistingProcessdoes not register, so the server has nothing to compare with and keeps the id based behaviour for it.What type of PR is it?
Bug Fix
What is the Jira issue?
How should this be tested?
RemoteInterpreterEventServerRegistrationTestcovers each row of the table with a realInterpreterSettingandManagedInterpreterGroup. With the sender check replaced by the previous behaviour, the three tests that expect the group to be kept fail and the three that expect it to be closed pass.RemoteInterpreterServerTest.testShutdownUnregistersWithItsHostAndPortchecks thatshutdown()unregisters with the process's host, port and group id.Run locally:
RemoteInterpreterEventServerRegistrationTest(6),RemoteInterpreterEventServerTest(5),RemoteInterpreterEventServerLibraryTest(6),RemoteInterpreterServerTest(4),InterpreterSettingTest(12),ManagedInterpreterGroupTest(2),InterpreterSettingManagerTest(13)RemoteInterpreterTest(17),RemoteInterpreterOutputTestStreamTest(4),RemoteAngularObjectTest(3),IdleInterpreterReclaimerTest(9),InterpreterFactoryTest(3)