Re: [PR] [KYUUBI #7301] Fix engine not killed after deregisterEngine [kyuubi]

["Copilot (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org