Skip to content

[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
apache:masterfrom
dev-donghwan:ZEPPELIN-6723
Open

dev-donghwan wants to merge 1 commit into
apache:masterfrom
dev-donghwan:ZEPPELIN-6723

Conversation

@dev-donghwan

Copy link
Copy Markdown
Contributor

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:

  • The server removes a group and then stops its process. A paragraph that runs in between creates a new group with the same id, and the old process's unregister closes it.
  • If the shutdown RPC fails while the process is alive, the process keeps its shutdown hook, is destroyed with SIGTERM a few seconds later, and the hook unregisters.
  • A process whose registration was rejected (see ZEPPELIN-6721) keeps running and unregisters when it exits later.

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 RegisterInfo with the unregister, records the RegisterInfo the server accepts at registration on the ManagedInterpreterGroup, and closes the group only when the two match:

Group found for the id Unregister carries Action
any no RegisterInfo (interpreter from before this change) close, as before
has an accepted registration the same RegisterInfo close, as before (ZEPPELIN-5140)
has an accepted registration a different RegisterInfo ignore, log a warning
no accepted registration, no process or a process still launching a RegisterInfo ignore: the group has no process that could send it
no accepted registration, a recovered or externally running process a RegisterInfo close, as before

Notes on the choices:

  • Compatibility. The RPC gets a second argument, unRegisterInterpreterProcess(1: string intpGroupId, 2: RegisterInfo registerInfo). The event service uses TBinaryProtocol, whose generated readers skip unknown fields, so an old server ignores the new argument and a new server receives null from an old interpreter, which keeps the id based behaviour.
  • Where the sender's RegisterInfo comes from. RemoteInterpreterServer builds it from its host, port and interpreterGroupId fields, which are set in the constructor and are what it registers with; registration and unregister now use the same getRegisterInfo(). It is not kept in RemoteInterpreterEventClient, because the client is replaced in init() and reconnect(). A DevInterpreter has no host and does not register, so it sends null.
  • What is compared. The server compares with the RegisterInfo it accepted, not with the process's host and port: K8sRemoteInterpreterProcess.processStarted() keeps localhost and a forwarded port, or the pod's DNS name, instead of the registered values.
  • Removal. The closed group is removed from its own setting by instance rather than by id from every setting, with the same 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.
  • Generated code. Regenerated with Thrift 0.13.0 through genthrift.sh. Only RemoteInterpreterEventService.java has real changes; the other generated files change only in their @Generated date, 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:

  • A recovered process or one attached through isExistingProcess does not register, so the server has nothing to compare with and keeps the id based behaviour for it.
  • Registration is also resolved by id only: a stray process that registers after a new group with the same id started its launch is accepted as that group's process. The server does not know a launched process's host and port before it registers, so this needs an identity given at launch, which ZEPPELIN-6607 plans for the gRPC runtime.

What type of PR is it?

Bug Fix

What is the Jira issue?

How should this be tested?

RemoteInterpreterEventServerRegistrationTest covers each row of the table with a real InterpreterSetting and ManagedInterpreterGroup. 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.testShutdownUnregistersWithItsHostAndPort checks that shutdown() 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)
  • With real interpreter processes: RemoteInterpreterTest (17), RemoteInterpreterOutputTestStreamTest (4), RemoteAngularObjectTest (3), IdleInterpreterReclaimerTest (9), InterpreterFactoryTest (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