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/10/16 10:37:08 UTC
[GitHub] [incubator-kyuubi] ChrisYu78 opened a new pull request, #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
ChrisYu78 opened a new pull request, #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637
### _Why are the changes needed?_
As described in issues#2887, random policy may cause task-hot-issues or production accident, so a SEQUENTIAL policy is added to avoid this problem
### _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
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 #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996425846
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefTests.scala:
##########
@@ -209,6 +209,17 @@ trait EngineRefTests extends KyuubiFunSuite {
conf.set(ENGINE_POOL_SIZE, 3)
val engine6 = new EngineRef(conf, user, id, null)
assert(engine6.subdomain.startsWith(s"$enginePoolName-"))
+
+ // unset subdomain and set engine pool name and 1 <= engine pool size < threshold
+ // and set ENGINE_POOL_BALANCE_POLICY
+ conf.unset(ENGINE_SHARE_LEVEL_SUBDOMAIN)
+ val enginePoolName7 = "test-pool"
+ conf.set(ENGINE_POOL_NAME, enginePoolName7)
+ conf.set(ENGINE_POOL_BALANCE_POLICY, "SEQUENTIAL")
+ conf.set(ENGINE_POOL_SIZE, 3)
+ val engine7 = new EngineRef(conf, user, id, null)
+ assert(engine7.subdomain.startsWith(s"$enginePoolName7-"))
+
Review Comment:
remove this blank
--
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 #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996425912
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -109,6 +99,35 @@ private[kyuubi] class EngineRef(
case _ => user
}
+ @VisibleForTesting
+ private[kyuubi] val subdomain: String = conf.get(ENGINE_SHARE_LEVEL_SUBDOMAIN) match {
+ case Some(_subdomain) => _subdomain
+ case None if clientPoolSize > 0 =>
+ val poolSize = math.min(clientPoolSize, poolThreshold)
+ if (poolSize < clientPoolSize) {
+ warn(s"Request engine pool size($clientPoolSize) exceeds, fallback to " +
+ s"system threshold $poolThreshold")
+ }
+ val seqNum =
+ if ("SEQUENTIAL".equals(enginePoolBalancePolicy)) {
+ info(s"The engine pool balance policy is SEQUENTIAL.")
+ val seqNumPath =
+ DiscoveryPaths.makePath(
+ s"${serverSpace}_$shareLevel",
+ "seq_num",
+ Array(appUser, clientPoolName))
+ withZkClient(conf) { zkClient =>
+ val dai = new DistributedAtomicInteger(zkClient, seqNumPath, new RetryForever(1000))
Review Comment:
```
val HA_CLIENT_CLASS: ConfigEntry[String] =
buildConf("kyuubi.ha.client.class")
.doc("Class name for service discovery client.<ul>" +
" <li>Zookeeper: org.apache.kyuubi.ha.client.zookeeper.ZookeeperDiscoveryClient</li>" +
" <li>Etcd: org.apache.kyuubi.ha.client.etcd.EtcdDiscoveryClient</li></ul>")
.version("1.6.0")
.stringConf
.checkValue(_.nonEmpty, "must not be empty")
.createWithDefault("org.apache.kyuubi.ha.client.zookeeper.ZookeeperDiscoveryClient")
```
Now for the ha client, it supports both zk and etcd
--
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] ChrisYu78 commented on a diff in pull request #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ChrisYu78 commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996433086
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/zookeeper/ZookeeperClientProvider.scala:
##########
@@ -144,4 +144,21 @@ object ZookeeperClientProvider extends Logging {
}
}
+ /**
+ * Creates a zookeeper client before calling `f` and close it after calling `f`.
+ */
+ def withZkClient[T](conf: KyuubiConf)(f: CuratorFramework => T): T = {
+ val zkClient = buildZookeeperClient(conf)
+ try {
+ zkClient.start()
+ f(zkClient)
+ } finally {
+ try {
+ zkClient.close()
+ } catch {
+ case e: IOException => error("Failed to release the zkClient", e)
+ }
+ }
+ }
Review Comment:
 Since the creation of DistributedAtomicInteger requires a client of CuratorFramework, the DiscoveryClient provided by DiscoveryClientProvider is converted into CuratorFramework
--
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 #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996426088
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/zookeeper/ZookeeperClientProvider.scala:
##########
@@ -144,4 +144,21 @@ object ZookeeperClientProvider extends Logging {
}
}
+ /**
+ * Creates a zookeeper client before calling `f` and close it after calling `f`.
+ */
+ def withZkClient[T](conf: KyuubiConf)(f: CuratorFramework => T): T = {
+ val zkClient = buildZookeeperClient(conf)
+ try {
+ zkClient.start()
+ f(zkClient)
+ } finally {
+ try {
+ zkClient.close()
+ } catch {
+ case e: IOException => error("Failed to release the zkClient", e)
+ }
+ }
+ }
Review Comment:
seems not needed, similar with DiscoveryClientProvider::withDiscoveryClient
```
object DiscoveryClientProvider extends Logging {
/**
* Creates a zookeeper client before calling `f` and close it after calling `f`.
*/
def withDiscoveryClient[T](conf: KyuubiConf)(f: DiscoveryClient => T): T = {
val discoveryClient = createDiscoveryClient(conf)
try {
discoveryClient.createClient()
f(discoveryClient)
} finally {
try {
discoveryClient.closeClient()
} catch {
case e: IOException => error("Failed to release the zkClient", e)
}
}
}
```
--
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 #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996426499
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -109,6 +99,35 @@ private[kyuubi] class EngineRef(
case _ => user
}
+ @VisibleForTesting
+ private[kyuubi] val subdomain: String = conf.get(ENGINE_SHARE_LEVEL_SUBDOMAIN) match {
+ case Some(_subdomain) => _subdomain
+ case None if clientPoolSize > 0 =>
+ val poolSize = math.min(clientPoolSize, poolThreshold)
+ if (poolSize < clientPoolSize) {
+ warn(s"Request engine pool size($clientPoolSize) exceeds, fallback to " +
+ s"system threshold $poolThreshold")
+ }
+ val seqNum =
+ if ("SEQUENTIAL".equals(enginePoolBalancePolicy)) {
Review Comment:
prefer pattern match
--
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 #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996425809
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -72,27 +75,14 @@ private[kyuubi] class EngineRef(
private val clientPoolName: String = conf.get(ENGINE_POOL_NAME)
+ private val enginePoolBalancePolicy: String = conf.get(ENGINE_POOL_BALANCE_POLICY)
+
// In case the multi kyuubi instances have the small gap of timeout, here we add
// a small amount of time for timeout
private val LOCK_TIMEOUT_SPAN_FACTOR = if (Utils.isTesting) 0.5 else 0.1
private var builder: ProcBuilder = _
- @VisibleForTesting
- private[kyuubi] val subdomain: String = conf.get(ENGINE_SHARE_LEVEL_SUBDOMAIN) match {
Review Comment:
unneeded change, keep the code the same location with before
--
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 #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996426262
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1385,6 +1385,13 @@ object KyuubiConf {
.intConf
.createWithDefault(-1)
+ val ENGINE_POOL_BALANCE_POLICY: ConfigEntry[String] = buildConf("engine.pool.balance.policy")
+ .doc("The balance policy of queries in engine pool.")
+ .version("1.7.0")
+ .stringConf
+ .checkValue(Set("RANDOM", "SEQUENTIAL").apply(_), "Unsupported balance policy")
Review Comment:
```
checkValues(Set("RANDOM", "SEQUENTIAL"))
```
or
```
checkValue(Set("RANDOM", "SEQUENTIAL").contains, "Unsupported balance policy")
```
--
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 #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei closed pull request #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
URL: https://github.com/apache/incubator-kyuubi/pull/3637
--
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 #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#issuecomment-1283650618
the new one https://github.com/apache/incubator-kyuubi/pull/3662
--
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 #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996426262
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1385,6 +1385,13 @@ object KyuubiConf {
.intConf
.createWithDefault(-1)
+ val ENGINE_POOL_BALANCE_POLICY: ConfigEntry[String] = buildConf("engine.pool.balance.policy")
+ .doc("The balance policy of queries in engine pool.")
+ .version("1.7.0")
+ .stringConf
+ .checkValue(Set("RANDOM", "SEQUENTIAL").apply(_), "Unsupported balance policy")
Review Comment:
```
.transform(_.map(_.toUpperCase(Locale.ROOT)))
.checkValues(Set("RANDOM", "SEQUENTIAL"))
```
or
```
.transform(_.map(_.toUpperCase(Locale.ROOT)))
.checkValue(Set("RANDOM", "SEQUENTIAL").contains, "Unsupported balance policy")
```
--
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 #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996426262
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1385,6 +1385,13 @@ object KyuubiConf {
.intConf
.createWithDefault(-1)
+ val ENGINE_POOL_BALANCE_POLICY: ConfigEntry[String] = buildConf("engine.pool.balance.policy")
+ .doc("The balance policy of queries in engine pool.")
+ .version("1.7.0")
+ .stringConf
+ .checkValue(Set("RANDOM", "SEQUENTIAL").apply(_), "Unsupported balance policy")
Review Comment:
```
.transform(_.toUpperCase(Locale.ROOT))
.checkValues(Set("RANDOM", "SEQUENTIAL"))
```
or
```
.transform(_.toUpperCase(Locale.ROOT))
.checkValue(Set("RANDOM", "SEQUENTIAL").contains, "Unsupported balance policy")
```
--
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] ChrisYu78 commented on a diff in pull request #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ChrisYu78 commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996430122
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -72,27 +75,14 @@ private[kyuubi] class EngineRef(
private val clientPoolName: String = conf.get(ENGINE_POOL_NAME)
+ private val enginePoolBalancePolicy: String = conf.get(ENGINE_POOL_BALANCE_POLICY)
+
// In case the multi kyuubi instances have the small gap of timeout, here we add
// a small amount of time for timeout
private val LOCK_TIMEOUT_SPAN_FACTOR = if (Utils.isTesting) 0.5 else 0.1
private var builder: ProcBuilder = _
- @VisibleForTesting
- private[kyuubi] val subdomain: String = conf.get(ENGINE_SHARE_LEVEL_SUBDOMAIN) match {
Review Comment:
Changed their order since the _subdomain_ uses the _appUser_ variable
--
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] ChrisYu78 commented on a diff in pull request #3637: [KYUUBI #2887] Add a sequential balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ChrisYu78 commented on code in PR #3637:
URL: https://github.com/apache/incubator-kyuubi/pull/3637#discussion_r996433086
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/zookeeper/ZookeeperClientProvider.scala:
##########
@@ -144,4 +144,21 @@ object ZookeeperClientProvider extends Logging {
}
}
+ /**
+ * Creates a zookeeper client before calling `f` and close it after calling `f`.
+ */
+ def withZkClient[T](conf: KyuubiConf)(f: CuratorFramework => T): T = {
+ val zkClient = buildZookeeperClient(conf)
+ try {
+ zkClient.start()
+ f(zkClient)
+ } finally {
+ try {
+ zkClient.close()
+ } catch {
+ case e: IOException => error("Failed to release the zkClient", e)
+ }
+ }
+ }
Review Comment:
 Since the creation of DistributedAtomicInteger requires a client of CuratorFramework, the DiscoveryClient provided by DiscoveryClientProvider is converted into CuratorFramework
--
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