[GitHub] [incubator-kyuubi] yanghua commented on a change in pull request #2064: [KYUUBI #2063] Fix engine idle timeout lose efficacy for Flink Engine

[GitBox <gi...@apache.org>]

yanghua commented on a change in pull request #2064:
URL: https://github.com/apache/incubator-kyuubi/pull/2064#discussion_r822238762



##########
File path: externals/kyuubi-flink-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/session/FlinkSQLSessionManager.scala
##########
@@ -76,6 +76,7 @@ class FlinkSQLSessionManager(engineContext: DefaultContext)
   }
 
   override def closeSession(sessionHandle: SessionHandle): Unit = {
+    super.closeSession(sessionHandle)
     executor.closeSession(sessionHandle.toString)

Review comment:
       > Should this add the check whether the ENGINE_SHARE_LEVEL config option is equal to ShareLevel.CONNECTION to stop the session that stops the current engine?
   
   I know this issue, Flink did not consider more about the engine share level. So I would prefer to rethink it in another thread. WDYT?
   
   > What's more, could you please add the test case to verify whether the bug is fixed?
   
   I have verified in my local. Writing test cases to test this issue is hard, we need to start kyuubi server, start flink local cluster then open a session and close it, and judge the engine process exited via fetching a specific log message.
   
   We may build a better test suite for the integration test in the future. WDYT?




-- 
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