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/19 08:36:01 UTC
[GitHub] [incubator-kyuubi] ChrisYu78 opened a new pull request, #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
ChrisYu78 opened a new pull request, #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662
<!--
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/CONTRIBUTING.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.
-->
As described in issues#2887, random policy may cause task-hot-issues or production accident, so a POLLING balance policy is added to avoid this problem, we can "set kyuubi.engine.pool.balance.policy = POLLING" to use it.
### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [] Add screenshots for manual tests if appropriate
- [x] [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] ChrisYu78 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ChrisYu78 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r999371588
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1394,6 +1394,18 @@ object KyuubiConf {
.intConf
.createWithDefault(-1)
+ val ENGINE_POOL_BALANCE_POLICY: ConfigEntry[String] =
+ buildConf("kyuubi.engine.pool.balance.policy")
+ .doc("The balance policy of queries in engine pool.Note that, " +
+ "Now,the engine pool balance policy only supports RANDOM or POLLING")
Review Comment:
Modified after reference
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000200208
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -109,6 +96,34 @@ 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 = enginePoolBalancePolicy match {
+ case "POLLING" =>
+ info(s"The engine pool balance policy is POLLING.")
Review Comment:
the message does not much help w/o identity information, please remove or supply more information like the session id, the calculated subdomain engine ref Id, etc.
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000201931
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1394,6 +1394,17 @@ object KyuubiConf {
.intConf
.createWithDefault(-1)
+ val ENGINE_POOL_BALANCE_POLICY: ConfigEntry[String] =
+ buildConf("kyuubi.engine.pool.balance.policy")
+ .doc("The balance policy of queries in engine pool.<ul>" +
+ " <li>RANDOM - Randomly use the engine in the pool (default)</li>" +
Review Comment:
please remove "(default)", we have a column to display default values in the docs page.
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000207794
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClient.scala:
##########
@@ -164,6 +169,13 @@ trait DiscoveryClient extends Logging {
basePath: String,
initData: String,
useProtection: Boolean = false): Unit
+
+ /**
+ * Atomically get an Int number and add one
+ * @param path the path of stored data
+ * @return the stored data under path
+ */
+ def getAndInc(path: String): Int
Review Comment:
Sorry for the previously immature suggestion. I checked the JDK API, `AtomicInteger` and `AtomicLong` name the method `getAndIncrement`, do you mind updating to align w/ them? And please mention the expected behavior for corner cases in the doc, e.g. what's the expected behavior if the path does not exist? what will happen if overflow?
--
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] pan3793 commented on pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#issuecomment-1284421236
A high-level comment, I checked your closed PR using `o.a.c...DistributedAtomicInteger`, I think it's a good idea, to make it decouple w/ zookeeper, we can add a method in
```
traits DiscoveryClient {
...
def getAndInc(path: String): Int
}
```
For Zookeeper, `DistributedAtomicInteger` can be used to implement, and for ETCD, explicitly lock and get then set may be required. We also got reported that some guys want a JDBC-based DiscoveryClient implementation, and implementing the method using SQL is not much complex.
--
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] ychris78 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ychris78 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1001340167
##########
kyuubi-ha/src/test/scala/org/apache/kyuubi/ha/client/DiscoveryClientTests.scala:
##########
@@ -150,4 +150,24 @@ trait DiscoveryClientTests extends KyuubiFunSuite {
assert(e.getMessage contains s"Timeout to lock on path [$lockPath]")
}
}
+
+ test("getAndIncrement method test") {
+ withDiscoveryClient(conf) { discoveryClient =>
+ (0 until 10).foreach { i =>
+ val ii = discoveryClient.getAndIncrement("/get_and_increment_test")
+ assert(i == ii)
+ }
+ }
+ }
+
+ test("setData method test") {
+ withDiscoveryClient(conf) { discoveryClient =>
+ val data = "abc";
+ val path = "/setData_test"
+ discoveryClient.create(path, "PERSISTENT")
+ discoveryClient.setData(path, data.getBytes)
+ val getSata = new String(discoveryClient.getData(path))
+ assert(data.equals(getSata))
Review Comment:
fixed
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000207794
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClient.scala:
##########
@@ -164,6 +169,13 @@ trait DiscoveryClient extends Logging {
basePath: String,
initData: String,
useProtection: Boolean = false): Unit
+
+ /**
+ * Atomically get an Int number and add one
+ * @param path the path of stored data
+ * @return the stored data under path
+ */
+ def getAndInc(path: String): Int
Review Comment:
Sorry for the previously immature suggestion. I checked the JDK API, `AtomicInteger` and `AtomicLong` name the method `getAndIncrement`, do you mind updating to align w/ them? And please mention the cornet behavior in the doc, e.g. what's the behavior if the path does not exist?
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClient.scala:
##########
@@ -164,6 +169,13 @@ trait DiscoveryClient extends Logging {
basePath: String,
initData: String,
useProtection: Boolean = false): Unit
+
+ /**
+ * Atomically get an Int number and add one
+ * @param path the path of stored data
+ * @return the stored data under path
+ */
+ def getAndInc(path: String): Int
Review Comment:
Sorry for the previously immature suggestion. I checked the JDK API, `AtomicInteger` and `AtomicLong` name the method `getAndIncrement`, do you mind updating to align w/ them? And please mention the cornet behavior in the doc, e.g. what's the expected behavior if the path does not exist?
--
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] ychris78 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ychris78 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1001573451
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/zookeeper/ZookeeperDiscoveryClient.scala:
##########
@@ -290,6 +296,11 @@ class ZookeeperDiscoveryClient(conf: KyuubiConf) extends DiscoveryClient {
secretNode.start()
}
+ def getAndIncrement(path: String): Int = {
+ val dai = new DistributedAtomicInteger(zkClient, path, new RetryForever(1000))
+ dai.increment().preValue().intValue()
Review Comment:
thanks for the correction, fixed @cfmcgrady
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ChrisYu78 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r999372433
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefTests.scala:
##########
@@ -209,6 +209,21 @@ trait EngineRefTests extends KyuubiFunSuite {
conf.set(ENGINE_POOL_SIZE, 3)
val engine6 = new EngineRef(conf, user, id, null)
assert(engine6.subdomain.startsWith(s"$enginePoolName-"))
+
+ // set(ENGINE_POOL_BALANCE_POLICY, "POLLING")
+ conf.unset(ENGINE_SHARE_LEVEL_SUBDOMAIN)
+ conf.set(ENGINE_POOL_SIZE, 5)
+ val pool_name = "pool_default_name"
+ conf.set(ENGINE_POOL_NAME, pool_name)
+ conf.set(HighAvailabilityConf.HA_NAMESPACE, "engine_test")
+ conf.set(HighAvailabilityConf.HA_ADDRESSES, getConnectString())
+ conf.set(ENGINE_POOL_BALANCE_POLICY, "POLLING")
+ (0 until (10)).foreach { i =>
+ val engine7 = new EngineRef(conf, user, id, null)
+ // print(engine7.subdomain +",i:" + i + "\n")
Review Comment:
removed
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefTests.scala:
##########
@@ -209,6 +209,21 @@ trait EngineRefTests extends KyuubiFunSuite {
conf.set(ENGINE_POOL_SIZE, 3)
val engine6 = new EngineRef(conf, user, id, null)
assert(engine6.subdomain.startsWith(s"$enginePoolName-"))
+
+ // set(ENGINE_POOL_BALANCE_POLICY, "POLLING")
Review Comment:
removed
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r999326311
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefTests.scala:
##########
@@ -209,6 +209,21 @@ trait EngineRefTests extends KyuubiFunSuite {
conf.set(ENGINE_POOL_SIZE, 3)
val engine6 = new EngineRef(conf, user, id, null)
assert(engine6.subdomain.startsWith(s"$enginePoolName-"))
+
+ // set(ENGINE_POOL_BALANCE_POLICY, "POLLING")
Review Comment:
remove it
--
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] cxzl25 commented on pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
cxzl25 commented on PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#issuecomment-1288538590
The title is a bit misleading, this policy should work for all engine types.
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1001288361
##########
kyuubi-ha/src/test/scala/org/apache/kyuubi/ha/client/DiscoveryClientTests.scala:
##########
@@ -150,4 +150,24 @@ trait DiscoveryClientTests extends KyuubiFunSuite {
assert(e.getMessage contains s"Timeout to lock on path [$lockPath]")
}
}
+
+ test("getAndIncrement method test") {
+ withDiscoveryClient(conf) { discoveryClient =>
+ (0 until 10).foreach { i =>
+ val ii = discoveryClient.getAndIncrement("/get_and_increment_test")
+ assert(i == ii)
+ }
+ }
+ }
+
+ test("setData method test") {
+ withDiscoveryClient(conf) { discoveryClient =>
+ val data = "abc";
+ val path = "/setData_test"
+ discoveryClient.create(path, "PERSISTENT")
+ discoveryClient.setData(path, data.getBytes)
+ val getSata = new String(discoveryClient.getData(path))
+ assert(data.equals(getSata))
Review Comment:
"Sata" typo? and equals could be replaced w/ == in scala
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ChrisYu78 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r999372143
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1394,6 +1394,18 @@ object KyuubiConf {
.intConf
.createWithDefault(-1)
+ val ENGINE_POOL_BALANCE_POLICY: ConfigEntry[String] =
+ buildConf("kyuubi.engine.pool.balance.policy")
+ .doc("The balance policy of queries in engine pool.Note that, " +
+ "Now,the engine pool balance policy only supports RANDOM or POLLING")
+ .version("1.7.0")
+ .stringConf
+ .transform(_.toUpperCase(Locale.ROOT))
+ .checkValue(
Review Comment:
Modified
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r999325437
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefTests.scala:
##########
@@ -209,6 +209,21 @@ trait EngineRefTests extends KyuubiFunSuite {
conf.set(ENGINE_POOL_SIZE, 3)
val engine6 = new EngineRef(conf, user, id, null)
assert(engine6.subdomain.startsWith(s"$enginePoolName-"))
+
+ // set(ENGINE_POOL_BALANCE_POLICY, "POLLING")
+ conf.unset(ENGINE_SHARE_LEVEL_SUBDOMAIN)
+ conf.set(ENGINE_POOL_SIZE, 5)
+ val pool_name = "pool_default_name"
+ conf.set(ENGINE_POOL_NAME, pool_name)
+ conf.set(HighAvailabilityConf.HA_NAMESPACE, "engine_test")
+ conf.set(HighAvailabilityConf.HA_ADDRESSES, getConnectString())
+ conf.set(ENGINE_POOL_BALANCE_POLICY, "POLLING")
+ (0 until (10)).foreach { i =>
+ val engine7 = new EngineRef(conf, user, id, null)
+ // print(engine7.subdomain +",i:" + i + "\n")
Review Comment:
remove it
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#issuecomment-1284980325
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662?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 [#3662](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (02a8311) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/a6832b7ab6e979e9680cad554130234871142f63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a6832b7) will **increase** coverage by `0.10%`.
> The diff coverage is `90.47%`.
```diff
@@ Coverage Diff @@
## master #3662 +/- ##
============================================
+ Coverage 51.94% 52.04% +0.10%
Complexity 13 13
============================================
Files 484 485 +1
Lines 27136 27235 +99
Branches 3784 3796 +12
============================================
+ Hits 14095 14174 +79
- Misses 11675 11686 +11
- Partials 1366 1375 +9
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/kyuubi/ha/client/DiscoveryClient.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9EaXNjb3ZlcnlDbGllbnQuc2NhbGE=) | `36.36% <ø> (ø)` | |
| [...ha/client/zookeeper/ZookeeperDiscoveryClient.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC96b29rZWVwZXIvWm9va2VlcGVyRGlzY292ZXJ5Q2xpZW50LnNjYWxh) | `69.88% <66.66%> (-0.06%)` | :arrow_down: |
| [...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9ldGNkL0V0Y2REaXNjb3ZlcnlDbGllbnQuc2NhbGE=) | `67.64% <90.00%> (+1.39%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvRW5naW5lUmVmLnNjYWxh) | `74.24% <90.90%> (+1.13%)` | :arrow_up: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662/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==) | `97.28% <100.00%> (-0.07%)` | :arrow_down: |
| [...yuubi/server/metadata/jdbc/JDBCMetadataStore.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbWV0YWRhdGEvamRiYy9KREJDTWV0YWRhdGFTdG9yZS5zY2FsYQ==) | `89.27% <0.00%> (-0.70%)` | :arrow_down: |
| [.../apache/kyuubi/server/api/v1/BatchesResource.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL3YxL0JhdGNoZXNSZXNvdXJjZS5zY2FsYQ==) | `68.91% <0.00%> (-0.64%)` | :arrow_down: |
| [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `80.74% <0.00%> (-0.63%)` | :arrow_down: |
| [...a/org/apache/kyuubi/service/TFrontendService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL1RGcm9udGVuZFNlcnZpY2Uuc2NhbGE=) | `91.17% <0.00%> (-0.30%)` | :arrow_down: |
| ... and [5 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/3662/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) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] ychris78 commented on pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ychris78 commented on PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#issuecomment-1288523742
> thanks @ychris78, merging to master
thanks, merged 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] turboFei closed pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei closed pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
URL: 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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r999324091
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -109,6 +96,47 @@ 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 = enginePoolBalancePolicy match {
+ case "POLLING" =>
+ info(s"The engine pool balance policy is POLLING.")
+ val snPath =
+ DiscoveryPaths.makePath(
+ s"${serverSpace}_$shareLevel",
+ "seq_num",
+ Array(appUser, clientPoolName))
Review Comment:
@pan3793 do you have some suggestion for the data path?
- it should contains kyuubi version
- it should contains engine type
- share level is not needed, because it is always user share level
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r999317858
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -109,6 +96,47 @@ 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 = enginePoolBalancePolicy match {
+ case "POLLING" =>
+ info(s"The engine pool balance policy is POLLING.")
+ val snPath =
+ DiscoveryPaths.makePath(
+ s"${serverSpace}_$shareLevel",
Review Comment:
s"${serverSpace}_${KYUUBI_VERSION}_$shareLevel",
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ChrisYu78 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000686394
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClient.scala:
##########
@@ -164,6 +169,14 @@ trait DiscoveryClient extends Logging {
basePath: String,
initData: String,
useProtection: Boolean = false): Unit
+
+ /**
+ * Atomically get an Int number and add one
+ * @param path the path of stored data,
+ * If the path does not exist, it will be created and initialized to 0
+ * @return the stored data under path
+ */
+ def getAndIncrement(path: String): Int
Review Comment:
done
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r999804288
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -109,6 +96,47 @@ 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 = enginePoolBalancePolicy match {
+ case "POLLING" =>
+ info(s"The engine pool balance policy is POLLING.")
+ val snPath =
+ DiscoveryPaths.makePath(
+ s"${serverSpace}_$shareLevel",
+ "seq_num",
+ Array(appUser, clientPoolName))
Review Comment:
the engine pool is designed to be orthogonal w/ the engine share level, it says we can combine the GROUP share level w/ the pool feature.
--
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] ychris78 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ychris78 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1001304041
##########
kyuubi-ha/src/test/scala/org/apache/kyuubi/ha/client/DiscoveryClientTests.scala:
##########
@@ -150,4 +150,24 @@ trait DiscoveryClientTests extends KyuubiFunSuite {
assert(e.getMessage contains s"Timeout to lock on path [$lockPath]")
}
}
+
+ test("getAndIncrement method test") {
+ withDiscoveryClient(conf) { discoveryClient =>
+ (0 until 10).foreach { i =>
+ val ii = discoveryClient.getAndIncrement("/get_and_increment_test")
+ assert(i == ii)
+ }
+ }
+ }
+
+ test("setData method test") {
+ withDiscoveryClient(conf) { discoveryClient =>
+ val data = "abc";
+ val path = "/setData_test"
+ discoveryClient.create(path, "PERSISTENT")
+ discoveryClient.setData(path, data.getBytes)
+ val getSata = new String(discoveryClient.getData(path))
+ assert(data.equals(getSata))
Review Comment:
yes, a typo
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000200208
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -109,6 +96,34 @@ 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 = enginePoolBalancePolicy match {
+ case "POLLING" =>
+ info(s"The engine pool balance policy is POLLING.")
Review Comment:
it does not help w/o a session id, please remove or supply more information like the session id, the calculated subdomain engine ref Id, etc.
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000200385
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -109,6 +96,34 @@ 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 = enginePoolBalancePolicy match {
+ case "POLLING" =>
+ info(s"The engine pool balance policy is POLLING.")
+ val snPath =
+ DiscoveryPaths.makePath(
+ s"${serverSpace}_${KYUUBI_VERSION}_${shareLevel}_$engineType",
+ "seq_num",
+ Array(appUser, clientPoolName))
+ DiscoveryClientProvider.withDiscoveryClient(conf) { client =>
+ client.getAndInc(snPath)
+ }
+ case "RANDOM" =>
+ info(s"The engine pool balance policy is RANDOM.")
Review Comment:
ditto
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000200208
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -109,6 +96,34 @@ 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 = enginePoolBalancePolicy match {
+ case "POLLING" =>
+ info(s"The engine pool balance policy is POLLING.")
Review Comment:
it does not help w/o a session id, please remove or supply more information like session id, engine ref Id etc.
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000207794
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClient.scala:
##########
@@ -164,6 +169,13 @@ trait DiscoveryClient extends Logging {
basePath: String,
initData: String,
useProtection: Boolean = false): Unit
+
+ /**
+ * Atomically get an Int number and add one
+ * @param path the path of stored data
+ * @return the stored data under path
+ */
+ def getAndInc(path: String): Int
Review Comment:
Sorry for the previously immature suggestion. I checked the JDK API, `AtomicInteger` and `AtomicLong` name the method `getAndIncrement`, do you mind updating to align w/ them? And please mention the expected behavior for corner cases in the doc, e.g. what's the expected behavior if the path does not exist?
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000207794
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClient.scala:
##########
@@ -164,6 +169,13 @@ trait DiscoveryClient extends Logging {
basePath: String,
initData: String,
useProtection: Boolean = false): Unit
+
+ /**
+ * Atomically get an Int number and add one
+ * @param path the path of stored data
+ * @return the stored data under path
+ */
+ def getAndInc(path: String): Int
Review Comment:
Sorry for the previously immature suggestion. I checked the JDK API, `AtomicInteger` and `AtomicLong` name the method `getAndIncrement`, do you mind updating to align w/ them? And please mention the cornering cases in the doc, e.g. what's the expected behavior if the path does not exist?
--
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] pan3793 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000539262
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClient.scala:
##########
@@ -164,6 +169,14 @@ trait DiscoveryClient extends Logging {
basePath: String,
initData: String,
useProtection: Boolean = false): Unit
+
+ /**
+ * Atomically get an Int number and add one
+ * @param path the path of stored data,
+ * If the path does not exist, it will be created and initialized to 0
+ * @return the stored data under path
+ */
+ def getAndIncrement(path: String): Int
Review Comment:
please add test case in `DiscoveryClientTests` to cover the added methods
--
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] pan3793 commented on pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#issuecomment-1285420505
thanks @ChrisYu78, overall lgtm
--
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] cfmcgrady commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
cfmcgrady commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1001527445
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/zookeeper/ZookeeperDiscoveryClient.scala:
##########
@@ -290,6 +296,11 @@ class ZookeeperDiscoveryClient(conf: KyuubiConf) extends DiscoveryClient {
secretNode.start()
}
+ def getAndIncrement(path: String): Int = {
+ val dai = new DistributedAtomicInteger(zkClient, path, new RetryForever(1000))
+ dai.increment().preValue().intValue()
Review Comment:
from class `DistributedAtomicInteger` Java API documentation:
> The various increment methods return an [AtomicValue](https://curator.apache.org/apidocs/org/apache/curator/framework/recipes/atomic/AtomicValue.html) object. You must always check [AtomicValue.succeeded()](https://curator.apache.org/apidocs/org/apache/curator/framework/recipes/atomic/AtomicValue.html#succeeded()). None of the methods (other than get()) are guaranteed to succeed.
we need to retry `dai.increment()`
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#issuecomment-1288472986
thanks @ychris78, 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] ychris78 commented on pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for engine pool
Posted by GitBox <gi...@apache.org>.
ychris78 commented on PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#issuecomment-1288565806
> The title is a bit misleading, this policy should work for all engine types.
thanks, corrected
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ChrisYu78 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000136933
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -109,6 +96,47 @@ 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 = enginePoolBalancePolicy match {
+ case "POLLING" =>
+ info(s"The engine pool balance policy is POLLING.")
+ val snPath =
+ DiscoveryPaths.makePath(
+ s"${serverSpace}_$shareLevel",
+ "seq_num",
+ Array(appUser, clientPoolName))
Review Comment:
changed to
DiscoveryPaths.makePath(
s"${serverSpace}_${KYUUBI_VERSION}_${shareLevel}_$engineType",
"seq_num",
Array(appUser, clientPoolName))
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r999304407
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1394,6 +1394,18 @@ object KyuubiConf {
.intConf
.createWithDefault(-1)
+ val ENGINE_POOL_BALANCE_POLICY: ConfigEntry[String] =
+ buildConf("kyuubi.engine.pool.balance.policy")
+ .doc("The balance policy of queries in engine pool.Note that, " +
+ "Now,the engine pool balance policy only supports RANDOM or POLLING")
+ .version("1.7.0")
+ .stringConf
+ .transform(_.toUpperCase(Locale.ROOT))
+ .checkValue(
Review Comment:
```
checkValues(Set("RANDOM", "POLLING"))
```
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#issuecomment-1283842702
```
--
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 closed pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ChrisYu78 closed pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
URL: 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] ChrisYu78 commented on a diff in pull request #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
ChrisYu78 commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r1000247754
##########
kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/DiscoveryClient.scala:
##########
@@ -164,6 +169,13 @@ trait DiscoveryClient extends Logging {
basePath: String,
initData: String,
useProtection: Boolean = false): Unit
+
+ /**
+ * Atomically get an Int number and add one
+ * @param path the path of stored data
+ * @return the stored data under path
+ */
+ def getAndInc(path: String): Int
Review Comment:
done
--
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 #3662: [KYUUBI #2887] Add a POLLING balance policy for spark engine pool
Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #3662:
URL: https://github.com/apache/incubator-kyuubi/pull/3662#discussion_r999307228
##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala:
##########
@@ -1394,6 +1394,18 @@ object KyuubiConf {
.intConf
.createWithDefault(-1)
+ val ENGINE_POOL_BALANCE_POLICY: ConfigEntry[String] =
+ buildConf("kyuubi.engine.pool.balance.policy")
+ .doc("The balance policy of queries in engine pool.Note that, " +
+ "Now,the engine pool balance policy only supports RANDOM or POLLING")
Review Comment:
```
val SASL_QOP: ConfigEntry[String] = buildConf("kyuubi.authentication.sasl.qop")
.doc("Sasl QOP enable higher levels of protection for Kyuubi communication with clients.<ul>" +
" <li>auth - authentication only (default)</li>" +
" <li>auth-int - authentication plus integrity protection</li>" +
" <li>auth-conf - authentication plus integrity and confidentiality protection. This is" +
" applicable only if Kyuubi is configured to use Kerberos authentication.</li> </ul>")
.version("1.0.0")
.stringConf
.checkValues(SaslQOP.values.map(_.toString))
.transform(_.toLowerCase(Locale.ROOT))
.createWithDefault(SaslQOP.AUTH.toString)
```
You can refer above config entry and explain the policy one by one.
--
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