pan3793 commented on code in PR #7777:
URL: https://github.com/apache/kyuubi/pull/7777#discussion_r4268478521
##########
kyuubi-ha/src/test/scala/org/apache/kyuubi/ha/client/DiscoveryClientTests.scala:
##########
@@ -129,6 +130,55 @@ trait DiscoveryClientTests extends KyuubiFunSuite {
}
}
+ test("Stop gracefully without waiting on sessions of a stopped session
manager") {
Review Comment:
The new case is skipped by `EtcdDiscoveryClientSuite`: inherited tests
register before `hasDockerEnv` is initialized, so the overridden `test` sees
`false`. Please make the Docker check initialization-safe, for example with
`lazy val`. This registration issue predates this PR, so fixing it in a
dedicated PR is also fine.
##########
kyuubi-ha/src/test/scala/org/apache/kyuubi/ha/client/DiscoveryClientTests.scala:
##########
@@ -129,6 +130,55 @@ trait DiscoveryClientTests extends KyuubiFunSuite {
}
}
+ test("Stop gracefully without waiting on sessions of a stopped session
manager") {
+ val namespace = "kyuubiengine-stopped-backend"
+
+ conf
+ .set(HA_ADDRESSES, getConnectString)
+ .set(HA_NAMESPACE, namespace)
+ .set(KyuubiConf.FRONTEND_THRIFT_BINARY_BIND_PORT, 0)
+
+ var discovery: ServiceDiscovery = null
+ val service: Serverable = new NoopTBinaryFrontendServer() {
+ override val frontendServices: Seq[NoopTBinaryFrontendService] = Seq(
+ new NoopTBinaryFrontendService(this) {
+ override val discoveryService: Option[Service] = {
+ discovery = new KyuubiServiceDiscovery(this)
+ Some(discovery)
+ }
+ })
+ }
+ service.initialize(conf)
+ service.start()
+
+ val basePath = s"/$namespace"
+ try {
+ val sessionManager = service.backendService.sessionManager
+ // simulate a session left open by a disconnected client
+ sessionManager.openSession(
+ TProtocolVersion.HIVE_CLI_SERVICE_PROTOCOL_V10,
+ "kyuubi",
+ "",
+ "localhost",
+ Map.empty)
+ sessionManager.stop()
+ assert(sessionManager.getActiveUserSessionCount === 1)
+
+ withDiscoveryClient(conf) { discoveryClient =>
+ discoveryClient.getChildren(basePath).foreach { child =>
+ discoveryClient.delete(s"$basePath/$child")
+ }
+ eventually(timeout(5.seconds), interval(100.millis)) {
+ assert(discovery.getServiceState === ServiceState.STOPPED)
+ assert(service.getServiceState === ServiceState.STOPPED)
+ }
+ }
+ } finally {
+ service.stop()
Review Comment:
Please close the fixture's sessions in `finally` before calling
`service.stop()`. If the original hang recurs, `eventually` times out, but the
new cleanup then waits indefinitely on `gracefulShutdownLatch`. Moving
`sessionManager` outside the `try` and closing its sessions during cleanup lets
the test report failure instead of hanging.
--
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]
