Copilot commented on code in PR #7302:
URL: https://github.com/apache/kyuubi/pull/7302#discussion_r2767745223
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -367,6 +367,18 @@ private[kyuubi] class EngineRef(
val msg = s"Deleting engine node:$sn"
info(msg)
discoveryClient.delete(s"$engineSpace/${sn.nodeName}")
+
+ if (shareLevel != CONNECTION && builder != null && engineManager
!= null) {
+ try {
+ val appMgrInfo = builder.appMgrInfo()
+ engineManager.killApplication(appMgrInfo, engineRefId,
Some(appUser))
+ info(s"Killed engine process for $engineRefId with share level
$shareLevel")
+ } catch {
+ case e: Exception =>
+ warn(s"Error killing engine process for $engineRefId", e)
+ }
+ }
Review Comment:
Critical bug: The condition `builder != null` will prevent killing engines
in the most common deregister scenarios. The `builder` field is only set when
this EngineRef instance creates a new engine (line 216 in the `create` method).
However, when an existing engine is discovered via `getOrCreate`, the `builder`
remains null. This means when deregister is called for discovered engines (the
exact scenario described in issue #7301), the engine process won't be killed.
Additionally, even if builder were non-null, calling `builder.appMgrInfo()`
is incorrect here because the appMgrInfo should be retrieved from the
ServiceNodeInfo that was just fetched from ZooKeeper. The correct pattern (as
shown in AdminResource.scala:312) is to get appMgrInfo from
`sn.attributes.get(KyuubiReservedKeys.KYUUBI_ENGINE_APP_MGR_INFO_KEY)`.
The fix should:
1. Get the ServiceNodeInfo from ZooKeeper
2. Extract appMgrInfo from `sn.attributes`
3. Use that to kill the application, without depending on the builder at all
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -367,6 +367,18 @@ private[kyuubi] class EngineRef(
val msg = s"Deleting engine node:$sn"
info(msg)
discoveryClient.delete(s"$engineSpace/${sn.nodeName}")
+
+ if (shareLevel != CONNECTION && builder != null && engineManager
!= null) {
+ try {
+ val appMgrInfo = builder.appMgrInfo()
+ engineManager.killApplication(appMgrInfo, engineRefId,
Some(appUser))
+ info(s"Killed engine process for $engineRefId with share level
$shareLevel")
+ } catch {
+ case e: Exception =>
+ warn(s"Error killing engine process for $engineRefId", e)
+ }
+ }
Review Comment:
The existing test for deregister (line 377-398 in EngineRefTests.scala)
doesn't verify that the engine process is actually killed when deregister is
called. The test only verifies that the engine is removed from ZooKeeper. Since
this PR's purpose is to fix the bug where engines are not killed after
deregistration, a test should be added to verify that killApplication is called
with the correct parameters when deregister succeeds. This could be done by
mocking the engineManager and verifying the killApplication call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
