You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/04/07 14:56:36 UTC
[GitHub] [incubator-kyuubi] lightning-L opened a new pull request, #2295: [KYUUBI #2250] Support to limit the spark engine max running time
lightning-L opened a new pull request, #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/contributions.html
2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
--
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
[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#discussion_r846075175
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -64,11 +71,53 @@ case class SparkSQLEngine(spark: SparkSession) extends Serverable("SparkSQLEngin
assert(currentEngine.isDefined)
currentEngine.get.stop()
})
+
+ startLifetimeTerminatingChecker(() => {
+ assert(currentEngine.isDefined)
+ currentEngine.get.stop()
+ })
+ }
+
+ override def stop(): Unit = synchronized {
+ super.stop()
+
+ shutdown = true
+ val shutdownTimeout = conf.get(ENGINE_EXEC_POOL_SHUTDOWN_TIMEOUT)
+ ThreadUtils.shutdown(lifetimeTerminatingChecker, Duration(shutdownTimeout, TimeUnit.MILLISECONDS))
}
override protected def stopServer(): Unit = {
countDownLatch.countDown()
}
+
+ private[kyuubi] def startLifetimeTerminatingChecker(stop: () => Unit): Unit = {
+ val interval = conf.get(ENGINE_CHECK_INTERVAL)
+ val maxLifetime = conf.get(ENGINE_SPARK_MAX_LIFETIME)
+ if (maxLifetime > 0) {
+ val checkTask = new Runnable {
+ override def run(): Unit = {
+ if (!shutdown && System.currentTimeMillis() - getStartTime > maxLifetime) {
+ if (!deregistered) {
+ info(s"Spark engine has been running for more than $maxLifetime ms," +
+ s"deregistering from engine discovery space.")
Review Comment:
nit:
```
s" deregistering
```
--
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
[GitHub] [incubator-kyuubi] turboFei closed pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei closed pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
URL: https://github.com/apache/incubator-kyuubi/pull/2295
--
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
[GitHub] [incubator-kyuubi] lightning-L commented on pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
lightning-L commented on PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#issuecomment-1092832143
> FYI:
>
> ```
> dev/reformat
> ```
>
> other command to check scala style
>
> ```
> ./build/mvn scalastyle:check
> ```
very useful. Thanks
--
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
[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#discussion_r845727114
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -64,11 +70,43 @@ case class SparkSQLEngine(spark: SparkSession) extends Serverable("SparkSQLEngin
assert(currentEngine.isDefined)
currentEngine.get.stop()
})
+
+ startLifetimeChecker(() => {
+ assert(currentEngine.isDefined)
+ currentEngine.get.stop()
+ })
+
+ }
+
+ override def stop(): Unit = synchronized {
+ super.stop()
+
+ shutdown = true
+ val shutdownTimeout: Long = conf.get(ENGINE_EXEC_POOL_SHUTDOWN_TIMEOUT)
+ ThreadUtils.shutdown(lifetimeChecker, Duration(shutdownTimeout, TimeUnit.MILLISECONDS))
}
override protected def stopServer(): Unit = {
countDownLatch.countDown()
}
+
+ private[kyuubi] def startLifetimeChecker(stop: () => Unit): Unit = {
+ val interval = conf.get(ENGINE_CHECK_INTERVAL)
+ val maxLifetime = conf.get(ENGINE_SPARK_MAX_LIFETIME)
+ if (maxLifetime > 0) {
+ val checkTask = new Runnable {
+ override def run(): Unit = {
+ val lifetime: Long = System.currentTimeMillis() - getStartTime
+ val openSessionCount: Int = backendService.sessionManager.getOpenSessionCount
+ if (!shutdown && lifetime > maxLifetime && openSessionCount <= 0) {
+ info(s"Spark engine has been running for more than $maxLifetime ms, terminating")
+ stop()
Review Comment:
We need deregister the engineServiceDiscovery at first.
And then if the current open session count is zero, we can stop the engine, otherwise, we need wait.
```
if (!shutdown && lifetime > maxLifetime) {
if (!deregistered) {
info (".... deregistering ...")
frontendServices.flatMap(_.discoveryService).map {
case engineServiceDiscovery: EngineServiceDiscovery =>
engineServiceDiscovery.stop()
}
deregistered=true
}
if (openSessionCount <= 0) {
info(s"Spark engine has been running for more than $maxLifetime ms, terminating")
stop()
}
}
```
--
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
[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#discussion_r845723230
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -64,11 +70,43 @@ case class SparkSQLEngine(spark: SparkSession) extends Serverable("SparkSQLEngin
assert(currentEngine.isDefined)
currentEngine.get.stop()
})
+
+ startLifetimeChecker(() => {
Review Comment:
maybe `startLifetimeTerminatingChecker` is better
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -622,6 +622,14 @@ object KyuubiConf {
.stringConf
.createOptional
+ val ENGINE_SPARK_MAX_LIFETIME: ConfigEntry[Long] =
+ buildConf("kyuubi.session.engine.spark.max.lifetime")
+ .doc("max lifetime for spark engine, the engine will self-terminate when it comes to the" +
Review Comment:
nit: max -> Max
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -622,6 +622,14 @@ object KyuubiConf {
.stringConf
.createOptional
+ val ENGINE_SPARK_MAX_LIFETIME: ConfigEntry[Long] =
+ buildConf("kyuubi.session.engine.spark.max.lifetime")
+ .doc("max lifetime for spark engine, the engine will self-terminate when it comes to the" +
+ " end of life. 0 or negative means not to self-terminate.")
+ .version("1.6.0")
+ .timeConf
+ .createWithDefault(Duration.ofHours(6).toMillis)
Review Comment:
by default it should be legacy `0` so that to align with original behavior
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/util/ThreadUtils.scala:
##########
@@ -64,4 +64,21 @@ object ThreadUtils extends Logging {
throw new KyuubiException("Exception thrown in awaitResult: ", e)
}
}
+
+ def shutdown(
+ executor: ExecutorService,
+ gracePeriod: Duration = FiniteDuration(30, TimeUnit.SECONDS)): Unit = {
Review Comment:
```
gracePeriod: Option[Duration] = None
```
So that:
```
gracePeriod.map { shutdownTimeout =>
executor.awaitTermination(shutdownTimeout, TimeUnit.MILLISECONDS)
}.getOrElse(executor.shutdown())
```
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -64,11 +70,43 @@ case class SparkSQLEngine(spark: SparkSession) extends Serverable("SparkSQLEngin
assert(currentEngine.isDefined)
currentEngine.get.stop()
})
+
+ startLifetimeChecker(() => {
+ assert(currentEngine.isDefined)
+ currentEngine.get.stop()
+ })
+
+ }
+
+ override def stop(): Unit = synchronized {
+ super.stop()
+
+ shutdown = true
+ val shutdownTimeout: Long = conf.get(ENGINE_EXEC_POOL_SHUTDOWN_TIMEOUT)
+ ThreadUtils.shutdown(lifetimeChecker, Duration(shutdownTimeout, TimeUnit.MILLISECONDS))
}
override protected def stopServer(): Unit = {
countDownLatch.countDown()
}
+
+ private[kyuubi] def startLifetimeChecker(stop: () => Unit): Unit = {
+ val interval = conf.get(ENGINE_CHECK_INTERVAL)
+ val maxLifetime = conf.get(ENGINE_SPARK_MAX_LIFETIME)
+ if (maxLifetime > 0) {
+ val checkTask = new Runnable {
+ override def run(): Unit = {
+ val lifetime: Long = System.currentTimeMillis() - getStartTime
+ val openSessionCount: Int = backendService.sessionManager.getOpenSessionCount
+ if (!shutdown && lifetime > maxLifetime && openSessionCount <= 0) {
+ info(s"Spark engine has been running for more than $maxLifetime ms, terminating")
+ stop()
Review Comment:
We need deregister the engineServiceDiscovery at first.
And then if the current open session count is zero, we can stop the engine, otherwise, we need wait.
```
if (!shutdown && lifetime > maxLifetime) {
if (unDeregistered) {
info (".... deregistering ...")
frontendServices.flatMap(_.discoveryService).map {
case engineServiceDiscovery: EngineServiceDiscovery =>
engineServiceDiscovery.stop()
}
}
if (openSessionCount <= 0) {
info(s"Spark engine has been running for more than $maxLifetime ms, terminating")
stop()
}
}
```
--
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
[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#discussion_r846055292
##########
docs/deployment/engine_lifecycle.md:
##########
@@ -60,6 +62,17 @@ The above two configurations can be used together to set the TTL of engines.
These configurations are user-facing and able to use in JDBC urls.
Note that, for [connection](engine_share_level.html#connection) share level engines that will be terminated at once when the connection is disconnected, these configurations not necessarily work in this case.
+### Engine max lifetime
+| Key | Default | Meaning | Type | Since |
+|----------------------------------------------|--------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------|--------------------------------------|
+| kyuubi\.session\.engine<br>\.spark\.max\.lifetime | <div style='width: 65pt;word-wrap: break-word;white-space: normal'>PT0s</div> | <div style='width: 170pt;word-wrap: break-word;white-space: normal'>Max lifetime for spark engine, the engine will self-terminate when it reaches the end of life. 0 or negative means not to self-terminate.</div> | <div style='width: 30pt'>duration</div> | <div style='width: 20pt'>1.6.0</div> |
+
+The max lifetime configuration only applies to spark engine currently.<br>
+For some use case, we might need restart the spark engine periodically. For example, to load the latest UDFs and jars.<br>
+If the spark engine has been running for a specified time, it need to deregister itself from engine discovery space(such as zookeeper) so that it will not receive new request.
+Then it will wait the current connection to close, and terminate itself.
Review Comment:
move to ### Engine TTL part?
--
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
[GitHub] [incubator-kyuubi] turboFei commented on pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#issuecomment-1092823977
FYI:
```
dev/reformat
```
--
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
[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#discussion_r846321632
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/kyuubi/engine/spark/ShareLevelSparkEngineSuite.scala:
##########
@@ -57,6 +65,25 @@ abstract class ShareLevelSparkEngineSuite
}
}
}
+
+ test("test spark engine max life-time") {
+ withZkClient { zkClient =>
+ assert(engine.getServiceState == ServiceState.STARTED)
+ assert(zkClient.checkExists().forPath(namespace) != null)
+ withJdbcStatement() { _ => }
+
+ eventually(Timeout(30.seconds)) {
+ shareLevel match {
+ case ShareLevel.CONNECTION =>
+ assert(engine.getServiceState == ServiceState.STOPPED)
+ assert(zkClient.checkExists().forPath(namespace) == null)
+ case _ =>
+ assert(engine.getServiceState == ServiceState.STOPPED)
+ assert(zkClient.checkExists().forPath(namespace) != null)
Review Comment:
@lightning-L Is it possible that?
```
ase _ =>
assert(engine.getServiceState == ServiceState.STOPPED)
assert(zkClient.checkExists().forPath(namespace) == null)
```
##########
externals/kyuubi-spark-sql-engine/src/test/scala/org/apache/kyuubi/engine/spark/ShareLevelSparkEngineSuite.scala:
##########
@@ -57,6 +65,25 @@ abstract class ShareLevelSparkEngineSuite
}
}
}
+
+ test("test spark engine max life-time") {
+ withZkClient { zkClient =>
+ assert(engine.getServiceState == ServiceState.STARTED)
+ assert(zkClient.checkExists().forPath(namespace) != null)
+ withJdbcStatement() { _ => }
+
+ eventually(Timeout(30.seconds)) {
+ shareLevel match {
+ case ShareLevel.CONNECTION =>
+ assert(engine.getServiceState == ServiceState.STOPPED)
+ assert(zkClient.checkExists().forPath(namespace) == null)
+ case _ =>
+ assert(engine.getServiceState == ServiceState.STOPPED)
+ assert(zkClient.checkExists().forPath(namespace) != null)
Review Comment:
@lightning-L Is it possible that?
```
case _ =>
assert(engine.getServiceState == ServiceState.STOPPED)
assert(zkClient.checkExists().forPath(namespace) == null)
```
--
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
[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#discussion_r846055292
##########
docs/deployment/engine_lifecycle.md:
##########
@@ -60,6 +62,17 @@ The above two configurations can be used together to set the TTL of engines.
These configurations are user-facing and able to use in JDBC urls.
Note that, for [connection](engine_share_level.html#connection) share level engines that will be terminated at once when the connection is disconnected, these configurations not necessarily work in this case.
+### Engine max lifetime
+| Key | Default | Meaning | Type | Since |
+|----------------------------------------------|--------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------|--------------------------------------|
+| kyuubi\.session\.engine<br>\.spark\.max\.lifetime | <div style='width: 65pt;word-wrap: break-word;white-space: normal'>PT0s</div> | <div style='width: 170pt;word-wrap: break-word;white-space: normal'>Max lifetime for spark engine, the engine will self-terminate when it reaches the end of life. 0 or negative means not to self-terminate.</div> | <div style='width: 30pt'>duration</div> | <div style='width: 20pt'>1.6.0</div> |
+
+The max lifetime configuration only applies to spark engine currently.<br>
+For some use case, we might need restart the spark engine periodically. For example, to load the latest UDFs and jars.<br>
+If the spark engine has been running for a specified time, it need to deregister itself from engine discovery space(such as zookeeper) so that it will not receive new request.
+Then it will wait the current connection to close, and terminate itself.
Review Comment:
move to `### Engine TTL` part?
--
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
[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#discussion_r845727114
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -64,11 +70,43 @@ case class SparkSQLEngine(spark: SparkSession) extends Serverable("SparkSQLEngin
assert(currentEngine.isDefined)
currentEngine.get.stop()
})
+
+ startLifetimeChecker(() => {
+ assert(currentEngine.isDefined)
+ currentEngine.get.stop()
+ })
+
+ }
+
+ override def stop(): Unit = synchronized {
+ super.stop()
+
+ shutdown = true
+ val shutdownTimeout: Long = conf.get(ENGINE_EXEC_POOL_SHUTDOWN_TIMEOUT)
+ ThreadUtils.shutdown(lifetimeChecker, Duration(shutdownTimeout, TimeUnit.MILLISECONDS))
}
override protected def stopServer(): Unit = {
countDownLatch.countDown()
}
+
+ private[kyuubi] def startLifetimeChecker(stop: () => Unit): Unit = {
+ val interval = conf.get(ENGINE_CHECK_INTERVAL)
+ val maxLifetime = conf.get(ENGINE_SPARK_MAX_LIFETIME)
+ if (maxLifetime > 0) {
+ val checkTask = new Runnable {
+ override def run(): Unit = {
+ val lifetime: Long = System.currentTimeMillis() - getStartTime
+ val openSessionCount: Int = backendService.sessionManager.getOpenSessionCount
+ if (!shutdown && lifetime > maxLifetime && openSessionCount <= 0) {
+ info(s"Spark engine has been running for more than $maxLifetime ms, terminating")
+ stop()
Review Comment:
We need deregister the engineServiceDiscovery at first.
And then if the current open session count is zero, we can stop the engine, otherwise, we need wait.
```
if (!shutdown && lifetime > maxLifetime) {
if (!deregistered) {
info (".... deregistering ...")
frontendServices.flatMap(_.discoveryService).map {
case engineServiceDiscovery: EngineServiceDiscovery =>
engineServiceDiscovery.stop()
}
deregistered=true
}
if (backendService.sessionManager.getOpenSessionCount <= 0) {
info(s"Spark engine has been running for more than $maxLifetime ms, terminating")
stop()
}
}
```
--
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
[GitHub] [incubator-kyuubi] turboFei commented on pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#issuecomment-1092942611
thanks, merging to master
--
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
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#issuecomment-1092418848
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2295](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c827e90) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/82a024a9837104bb74d6ea9ba103439794b84232?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (82a024a) will **increase** coverage by `0.02%`.
> The diff coverage is `80.55%`.
```diff
@@ Coverage Diff @@
## master #2295 +/- ##
============================================
+ Coverage 62.25% 62.27% +0.02%
Complexity 69 69
============================================
Files 353 353
Lines 16757 16785 +28
Branches 2269 2272 +3
============================================
+ Hits 10432 10453 +21
- Misses 5366 5370 +4
- Partials 959 962 +3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ain/scala/org/apache/kyuubi/util/ThreadUtils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS91dGlsL1RocmVhZFV0aWxzLnNjYWxh) | `86.20% <50.00%> (-13.80%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9TcGFya1NRTEVuZ2luZS5zY2FsYQ==) | `76.72% <85.71%> (+1.36%)` | :arrow_up: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.34% <100.00%> (+0.02%)` | :arrow_up: |
| [...ala/org/apache/kyuubi/session/SessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL1Nlc3Npb25NYW5hZ2VyLnNjYWxh) | `87.50% <100.00%> (+1.78%)` | :arrow_up: |
| [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `67.46% <0.00%> (-2.41%)` | :arrow_down: |
| [.../org/apache/kyuubi/engine/hive/HiveSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1oaXZlLXNxbC1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL2hpdmUvSGl2ZVNRTEVuZ2luZS5zY2FsYQ==) | `70.00% <0.00%> (-1.67%)` | :arrow_down: |
| [...a/org/apache/kyuubi/plugin/SessionConfAdvisor.java](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2t5dXViaS9wbHVnaW4vU2Vzc2lvbkNvbmZBZHZpc29yLmphdmE=) | | |
| [...pache/kyuubi/plugin/DefaultSessionConfAdvisor.java](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2t5dXViaS9wbHVnaW4vRGVmYXVsdFNlc3Npb25Db25mQWR2aXNvci5qYXZh) | | |
| [...pache/kyuubi/plugin/DefaultSessionConfAdvisor.java](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9reXV1Ymktc2VydmVyL2t5dXViaS1zZXJ2ZXItcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL0RlZmF1bHRTZXNzaW9uQ29uZkFkdmlzb3IuamF2YQ==) | `100.00% <0.00%> (ø)` | |
| ... and [1 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [82a024a...c827e90](https://codecov.io/gh/apache/incubator-kyuubi/pull/2295?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [incubator-kyuubi] lightning-L commented on a diff in pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
lightning-L commented on code in PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#discussion_r845969698
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/util/ThreadUtils.scala:
##########
@@ -64,4 +64,21 @@ object ThreadUtils extends Logging {
throw new KyuubiException("Exception thrown in awaitResult: ", e)
}
}
+
+ def shutdown(
+ executor: ExecutorService,
+ gracePeriod: Duration = FiniteDuration(30, TimeUnit.SECONDS)): Unit = {
Review Comment:
This is the refactor for original code in SessionManager.scala#stop().
And I also refered to the ThreadUtils.scala#shutdown in spark code, which has the default 30s for gracePeriod.
To shut down the ExecutorService, we use shutdown() method combined with awaitTermination() method. With this approach, the ExecutorService will first stop taking new tasks and then wait up to a specified period of time for all tasks to be completed. If that time expires, the execution is stopped immediately.
--
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
[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#discussion_r846055292
##########
docs/deployment/engine_lifecycle.md:
##########
@@ -60,6 +62,17 @@ The above two configurations can be used together to set the TTL of engines.
These configurations are user-facing and able to use in JDBC urls.
Note that, for [connection](engine_share_level.html#connection) share level engines that will be terminated at once when the connection is disconnected, these configurations not necessarily work in this case.
+### Engine max lifetime
+| Key | Default | Meaning | Type | Since |
+|----------------------------------------------|--------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------|--------------------------------------|
+| kyuubi\.session\.engine<br>\.spark\.max\.lifetime | <div style='width: 65pt;word-wrap: break-word;white-space: normal'>PT0s</div> | <div style='width: 170pt;word-wrap: break-word;white-space: normal'>Max lifetime for spark engine, the engine will self-terminate when it reaches the end of life. 0 or negative means not to self-terminate.</div> | <div style='width: 30pt'>duration</div> | <div style='width: 20pt'>1.6.0</div> |
+
+The max lifetime configuration only applies to spark engine currently.<br>
+For some use case, we might need restart the spark engine periodically. For example, to load the latest UDFs and jars.<br>
+If the spark engine has been running for a specified time, it need to deregister itself from engine discovery space(such as zookeeper) so that it will not receive new request.
+Then it will wait the current connection to close, and terminate itself.
Review Comment:
move to `### Engine TTL` part?
--
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
[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2295: [KYUUBI #2250] Support to limit the spark engine max running time
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2295:
URL: https://github.com/apache/incubator-kyuubi/pull/2295#discussion_r846045041
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -64,11 +71,51 @@ case class SparkSQLEngine(spark: SparkSession) extends Serverable("SparkSQLEngin
assert(currentEngine.isDefined)
currentEngine.get.stop()
})
+
+ startLifetimeTerminatingChecker(() => {
+ assert(currentEngine.isDefined)
+ currentEngine.get.stop()
+ })
+
+ }
+
+ override def stop(): Unit = synchronized {
+ super.stop()
+
+ shutdown = true
+ val shutdownTimeout: Long = conf.get(ENGINE_EXEC_POOL_SHUTDOWN_TIMEOUT)
+ ThreadUtils.shutdown(lifetimeChecker, Duration(shutdownTimeout, TimeUnit.MILLISECONDS))
}
override protected def stopServer(): Unit = {
countDownLatch.countDown()
}
+
+ private[kyuubi] def startLifetimeTerminatingChecker(stop: () => Unit): Unit = {
+ val interval = conf.get(ENGINE_CHECK_INTERVAL)
+ val maxLifetime = conf.get(ENGINE_SPARK_MAX_LIFETIME)
+ if (maxLifetime > 0) {
+ val checkTask = new Runnable {
+ override def run(): Unit = {
+ if (!shutdown && System.currentTimeMillis() - getStartTime > maxLifetime) {
+ if (!deregistered) {
+ info("Deregistering spark engine ...")
+ frontendServices.flatMap(_.discoveryService).map {
+ case engineServiceDiscovery: EngineServiceDiscovery => engineServiceDiscovery.stop()
+ }
+ deregistered = true
+ }
+
+ if (backendService.sessionManager.getOpenSessionCount <= 0) {
+ info(s"Spark engine has been running for more than $maxLifetime ms, terminating")
Review Comment:
```
info(s"Spark engine has been running for more than $maxLifetime ms and no open session now, terminating")
```
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -64,11 +71,51 @@ case class SparkSQLEngine(spark: SparkSession) extends Serverable("SparkSQLEngin
assert(currentEngine.isDefined)
currentEngine.get.stop()
})
+
+ startLifetimeTerminatingChecker(() => {
+ assert(currentEngine.isDefined)
+ currentEngine.get.stop()
+ })
+
+ }
+
+ override def stop(): Unit = synchronized {
+ super.stop()
+
+ shutdown = true
+ val shutdownTimeout: Long = conf.get(ENGINE_EXEC_POOL_SHUTDOWN_TIMEOUT)
+ ThreadUtils.shutdown(lifetimeChecker, Duration(shutdownTimeout, TimeUnit.MILLISECONDS))
}
override protected def stopServer(): Unit = {
countDownLatch.countDown()
}
+
+ private[kyuubi] def startLifetimeTerminatingChecker(stop: () => Unit): Unit = {
+ val interval = conf.get(ENGINE_CHECK_INTERVAL)
+ val maxLifetime = conf.get(ENGINE_SPARK_MAX_LIFETIME)
+ if (maxLifetime > 0) {
+ val checkTask = new Runnable {
+ override def run(): Unit = {
+ if (!shutdown && System.currentTimeMillis() - getStartTime > maxLifetime) {
+ if (!deregistered) {
+ info("Deregistering spark engine ...")
Review Comment:
it is better that log more useful information.
likes
```
info(s"Spark engine has been running for more than $maxLifetime ms, deregistering from engine discovery space.")
```
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -64,11 +71,51 @@ case class SparkSQLEngine(spark: SparkSession) extends Serverable("SparkSQLEngin
assert(currentEngine.isDefined)
currentEngine.get.stop()
})
+
+ startLifetimeTerminatingChecker(() => {
+ assert(currentEngine.isDefined)
+ currentEngine.get.stop()
+ })
+
+ }
+
+ override def stop(): Unit = synchronized {
+ super.stop()
+
+ shutdown = true
+ val shutdownTimeout: Long = conf.get(ENGINE_EXEC_POOL_SHUTDOWN_TIMEOUT)
Review Comment:
nit: the `: Long` is not needed.
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/SparkSQLEngine.scala:
##########
@@ -64,11 +71,51 @@ case class SparkSQLEngine(spark: SparkSession) extends Serverable("SparkSQLEngin
assert(currentEngine.isDefined)
currentEngine.get.stop()
})
+
+ startLifetimeTerminatingChecker(() => {
+ assert(currentEngine.isDefined)
+ currentEngine.get.stop()
+ })
+
Review Comment:
nit: remove blank line
--
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