You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kyuubi.apache.org by GitBox <gi...@apache.org> on 2021/08/22 14:22:50 UTC
[GitHub] [incubator-kyuubi] timothy65535 opened a new pull request #974: [KYUUBI #962] Support multiple engines under USER share level
timothy65535 opened a new pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974
# Motivation
Apache Kyuubi provides three engine share levels:
- CONNECTION: engine will not be shared but only used by the current client connection
- SERVER: the App will be shared by Kyuubi servers
- USER: engine will be shared by all sessions created by a unique username
In the current design, kyuubi can provides engine pool through `kyuubi.engine.share.level.sub.domain` option for same user under `USER` share level. It does not have an auto-mechanisn, e.g. create two more engines under same `subDomain`, random picking engines. The goal of this proposal is to provide such auto-mechanism to creating and picking subDomain to implement the feature of the engine pool.
# Implementation
In this section, describe the implementation of Kyuubi Engine Pool. We need to consider several things
- How to control the concurrent create the engine?
- How to share the pool size over two more kyuubi server?
- Engine pick strategy(sequentially / randomly / load-based)?
The key point is to set engine pool size as the content of engine space znode when create the engine space firstlly, so that we can share the pool size over two more kyuubi server, and avoid the inconsistent configuration between different servers effectively.
## Design
### 1. High level
### 2. Detail work flow
# Tests
- KyuubiOperationPerConnectionSuite
- KyuubiOperationPerServerSuite
- KyuubiOperationPerUserSuite
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1e456f4) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.57%`.
> The diff coverage is `46.00%`.
> :exclamation: Current head 1e456f4 differs from pull request most recent head 25c332b. Consider uploading reports for the commit 25c332b to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.46% +0.57%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 644 -5
============================================
+ Hits 4358 4369 +11
+ Misses 717 682 -35
+ Partials 380 379 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `46.66% <0.00%> (+0.32%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...-common/src/main/scala-2.12/scala/util/Using.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS0yLjEyL3NjYWxhL3V0aWwvVXNpbmcuc2NhbGE=) | | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...25c332b](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693687430
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
When creating engine, will check `pool.size` and `engineNum`.
_`pool size`_: read from engine space.
_`engine num`_: children of engine space.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694227733
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -125,12 +128,34 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
}
- private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ def getOrCreate(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
+
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
+ val notFull = checkEnginePoolCapacity(zkClient, engineSpace)
+ if (notFull) {
+ createInternal(zkClient)
+ } else {
+ val engineRef = getEngineByPolicy(zkClient, engineSpace, providePolicy)
+ if (engineRef.nonEmpty) return engineRef.get
Review comment:
replace by pattern
```
engineRefOpt match {
case Some(engineRef) => ...
case None => ...
}
```
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694163349
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
For most scene, we use sparksql as offline analysis, `engine pool` is useful for us. Ok, if user very case about the time cost, keep `kyuubi.ha.engine.pool.size=1`, the kyuubi server will work like 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d16df38) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.57%`.
> The diff coverage is `47.05%`.
> :exclamation: Current head d16df38 differs from pull request most recent head 1e456f4. Consider uploading reports for the commit 1e456f4 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.46% +0.57%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 644 -5
============================================
+ Hits 4358 4369 +11
+ Misses 717 682 -35
+ Partials 380 379 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `46.66% <0.00%> (+0.32%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0Fic3RyYWN0U2Vzc2lvbi5zY2FsYQ==) | `95.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...1e456f4](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694181319
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
I don't get 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d16df38) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.57%`.
> The diff coverage is `47.05%`.
> :exclamation: Current head d16df38 differs from pull request most recent head 25c332b. Consider uploading reports for the commit 25c332b to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.46% +0.57%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 644 -5
============================================
+ Hits 4358 4369 +11
+ Misses 717 682 -35
+ Partials 380 379 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `46.66% <0.00%> (+0.32%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0Fic3RyYWN0U2Vzc2lvbi5zY2FsYQ==) | `95.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...25c332b](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694227733
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -125,12 +128,34 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
}
- private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ def getOrCreate(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
+
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
+ val notFull = checkEnginePoolCapacity(zkClient, engineSpace)
+ if (notFull) {
+ createInternal(zkClient)
+ } else {
+ val engineRef = getEngineByPolicy(zkClient, engineSpace, providePolicy)
+ if (engineRef.nonEmpty) return engineRef.get
Review comment:
rewrite it use pattern
```
engineRefOpt match {
case Some(engineRef) => ...
case None => ...
}
```
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693708959
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/engine/ProvidePolicy.scala
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+package org.apache.kyuubi.engine
+
+object ProvidePolicy extends Enumeration {
+
+ type ProvidePolicy = Value
+
+ val
+ /** A random policy that pick engines randomly. */
+ RANDOM = Value
Review comment:
Yeah, we can add other policy later.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693796916
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
So, the pool actually makes connect 2 a very worse case
1) it always needs to create a new engine even there is an available engine there. the time cost is much higher than just reusing.
2) it will fail if it fails to create a new engine. The failure risk is much higher to create more engines.
3) if we can fall back to reusing after failures in 2), the time cost is inevitable.
If the failure happens, connection 3 seems very like to repeat what connection 2 does.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694219046
##########
File path: dev/dependencyList
##########
@@ -29,6 +29,8 @@ htrace-core4/4.1.0-incubating//htrace-core4-4.1.0-incubating.jar
jackson-annotations/2.11.4//jackson-annotations-2.11.4.jar
jackson-core/2.11.4//jackson-core-2.11.4.jar
jackson-databind/2.11.4//jackson-databind-2.11.4.jar
+jackson-module-paranamer/2.11.4//jackson-module-paranamer-2.11.4.jar
Review comment:
Please open the file `LICENSE-binary` and scroll down to the bottom, we also need to list the deps there.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4e519e0) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.57%`.
> The diff coverage is `42.30%`.
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.46% +0.57%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 642 -7
============================================
+ Hits 4358 4369 +11
+ Misses 717 683 -34
+ Partials 380 378 -2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `18.42% <0.00%> (-35.43%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `48.71% <ø> (+2.37%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.33% <84.61%> (+1.21%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.14% <91.66%> (-1.14%)` | :arrow_down: |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `47.36% <0.00%> (-2.64%)` | :arrow_down: |
| [...-common/src/main/scala-2.12/scala/util/Using.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS0yLjEyL3NjYWxhL3V0aWwvVXNpbmcuc2NhbGE=) | | |
| [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-ZGV2L2t5dXViaS1leHRlbnNpb24tc3BhcmstMy0xL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL3NxbC9LeXV1YmlRdWVyeVN0YWdlUHJlcGFyYXRpb24uc2NhbGE=) | `80.39% <0.00%> (+0.98%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...4e519e0](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694172287
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerUserSuite.scala
##########
@@ -30,29 +33,118 @@ class KyuubiOperationPerUserSuite extends WithKyuubiServer with JDBCTests {
KyuubiConf().set(KyuubiConf.ENGINE_SHARE_LEVEL, "user")
}
- test("ensure two connections in user mode share the same engine") {
- var r1: String = null
- var r2: String = null
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r1 = res.getString("value")
+ test("ensure two connections share the same engine when engine pool size is 1.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "1",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "aaa"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r2 = res.getString("value")
+ assert(r1 === r2)
+ }
+ }
+
+ test("ensure two connections don't share the same engine when engine pool is 2.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "2",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "bbb",
+ KyuubiConf.ENGINE_IDLE_TIMEOUT.key -> "30000"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- eventually(timeout(120.seconds), interval(100.milliseconds)) {
- assert(r1 != null && r2 != null)
+ assert(r1 != r2)
}
+ }
- assert(r1 === r2)
+ test("ensure two of three connections share the same engine when engine pool size is 2.") {
Review comment:
> Can we add another case that connection 2, 3 concurrently created while connection 1's engine creation is finished
hi @yaooqinn, what's the aim of the test case? what will we assert?
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694218244
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
##########
@@ -108,4 +109,20 @@ object HighAvailabilityConf {
.version("1.4.0")
.stringConf
.createOptional
+
+ val HA_ZK_ENGINE_POOL_SIZE: ConfigEntry[Int] =
+ buildConf("ha.engine.pool.size")
+ .doc("Maximum number of engines when using USER share level.")
+ .version("1.4.0")
+ .intConf
+ .checkValue(s => s > 0 && s < 50, "Invalid Engine Pool Size.")
+ .createWithDefault(1)
+
+ val HA_ZK_ENGINE_PROVIDE_POLICY: ConfigEntry[String] =
+ buildConf("ha.engine.provide.policy")
+ .doc("Multiple engines can be exposed under same space when using USER share level, " +
+ "choose the appropriate strategy according to different scenarios.")
+ .version("1.4.0")
+ .stringConf
+ .createWithDefault(ProvidePolicy.RANDOM.toString)
Review comment:
I think you missed `.checkValues(ProvidePolicy.values.map(_.toString))`
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694641651
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -125,12 +128,34 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
}
- private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ def getOrCreate(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
Review comment:
When support two more engines under same space, we‘ll face concurrency issues. Had tried serveral ways to improve the lock, but failed. Clients will wait for the lock only when connect to the kyuubi server, and these connections are long-time connections.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694174091
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerUserSuite.scala
##########
@@ -30,29 +33,118 @@ class KyuubiOperationPerUserSuite extends WithKyuubiServer with JDBCTests {
KyuubiConf().set(KyuubiConf.ENGINE_SHARE_LEVEL, "user")
}
- test("ensure two connections in user mode share the same engine") {
- var r1: String = null
- var r2: String = null
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r1 = res.getString("value")
+ test("ensure two connections share the same engine when engine pool size is 1.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "1",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "aaa"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r2 = res.getString("value")
+ assert(r1 === r2)
+ }
+ }
+
+ test("ensure two connections don't share the same engine when engine pool is 2.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "2",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "bbb",
+ KyuubiConf.ENGINE_IDLE_TIMEOUT.key -> "30000"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- eventually(timeout(120.seconds), interval(100.milliseconds)) {
- assert(r1 != null && r2 != null)
+ assert(r1 != r2)
}
+ }
- assert(r1 === r2)
+ test("ensure two of three connections share the same engine when engine pool size is 2.") {
Review comment:
we will exactly create 2 engines and one for 2 connections and the other for 1 connection
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6aad2cc) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.57%`.
> The diff coverage is `47.05%`.
> :exclamation: Current head 6aad2cc differs from pull request most recent head 2ff39b9. Consider uploading reports for the commit 2ff39b9 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.46% +0.57%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 644 -5
============================================
+ Hits 4358 4369 +11
+ Misses 717 681 -36
Partials 380 380
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `47.50% <0.00%> (+1.15%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0Fic3RyYWN0U2Vzc2lvbi5zY2FsYQ==) | `95.50% <100.00%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `47.36% <0.00%> (-2.64%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...2ff39b9](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694163349
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
For most scene, we use sparksql as offline analysis, `engine pool` is useful for these scene. Ok, if user very case about the time cost, keep `kyuubi.ha.engine.pool.size=1`, the kyuubi server will work like 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694202864
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerUserSuite.scala
##########
@@ -30,29 +33,118 @@ class KyuubiOperationPerUserSuite extends WithKyuubiServer with JDBCTests {
KyuubiConf().set(KyuubiConf.ENGINE_SHARE_LEVEL, "user")
}
- test("ensure two connections in user mode share the same engine") {
- var r1: String = null
- var r2: String = null
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r1 = res.getString("value")
+ test("ensure two connections share the same engine when engine pool size is 1.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "1",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "aaa"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r2 = res.getString("value")
+ assert(r1 === r2)
+ }
+ }
+
+ test("ensure two connections don't share the same engine when engine pool is 2.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "2",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "bbb",
+ KyuubiConf.ENGINE_IDLE_TIMEOUT.key -> "30000"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- eventually(timeout(120.seconds), interval(100.milliseconds)) {
- assert(r1 != null && r2 != null)
+ assert(r1 != r2)
}
+ }
- assert(r1 === r2)
+ test("ensure two of three connections share the same engine when engine pool size is 2.") {
Review comment:
Had updated the test case, correct me if wrong.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694216809
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
##########
@@ -108,4 +109,20 @@ object HighAvailabilityConf {
.version("1.4.0")
.stringConf
.createOptional
+
+ val HA_ZK_ENGINE_POOL_SIZE: ConfigEntry[Int] =
+ buildConf("ha.engine.pool.size")
+ .doc("Maximum number of engines when using USER share level.")
+ .version("1.4.0")
+ .intConf
+ .checkValue(s => s > 0 && s < 50, "Invalid Engine Pool Size.")
Review comment:
Good !!!
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
##########
@@ -108,4 +109,20 @@ object HighAvailabilityConf {
.version("1.4.0")
.stringConf
.createOptional
+
+ val HA_ZK_ENGINE_POOL_SIZE: ConfigEntry[Int] =
+ buildConf("ha.engine.pool.size")
+ .doc("Maximum number of engines when using USER share level.")
+ .version("1.4.0")
+ .intConf
+ .checkValue(s => s > 0 && s < 50, "Invalid Engine Pool Size.")
+ .createWithDefault(1)
+
+ val HA_ZK_ENGINE_PROVIDE_POLICY: ConfigEntry[String] =
+ buildConf("ha.engine.provide.policy")
+ .doc("Multiple engines can be exposed under same space when using USER share level, " +
+ "choose the appropriate strategy according to different scenarios.")
+ .version("1.4.0")
+ .stringConf
+ .createWithDefault(ProvidePolicy.RANDOM.toString)
Review comment:
👍
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693898618
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerUserSuite.scala
##########
@@ -30,29 +31,68 @@ class KyuubiOperationPerUserSuite extends WithKyuubiServer with JDBCTests {
KyuubiConf().set(KyuubiConf.ENGINE_SHARE_LEVEL, "user")
}
- test("ensure two connections in user mode share the same engine") {
- var r1: String = null
- var r2: String = null
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r1 = res.getString("value")
- }
- }.start()
+ test("ensure two connections share the same engine when engine pool size is 1.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "1",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "aaa"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r2 = res.getString("value")
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- eventually(timeout(120.seconds), interval(100.milliseconds)) {
- assert(r1 != null && r2 != null)
+ assert(r1 === r2)
}
+ }
+
+ test("ensure two connections don't share the same engine when engine pool size bigger than 1.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "2",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "bbb"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
- assert(r1 === r2)
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
+ }
+
+ assert(r1 != r2)
Review comment:
make sense, had updated.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694163349
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
For most scene, we use sparksql as offline analysis, `engine pool` is useful for these scene. Ok, if user very care about the time cost, keep `kyuubi.ha.engine.pool.size=1`, the kyuubi server will work like 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693633499
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
How can we reach to pool size？
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693690010
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
can you add a test to verify this？
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694229502
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -125,12 +128,34 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
}
- private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ def getOrCreate(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
+
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
+ val notFull = checkEnginePoolCapacity(zkClient, engineSpace)
+ if (notFull) {
+ createInternal(zkClient)
+ } else {
+ val engineRef = getEngineByPolicy(zkClient, engineSpace, providePolicy)
+ if (engineRef.nonEmpty) return engineRef.get
+ else throw new Exception("Unexpected exception!!!")
Review comment:
Avoid using raw `Exception`, pls pick a specific `KyuubiException` or create a new subclass if there is no suitable 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694212846
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
##########
@@ -108,4 +109,20 @@ object HighAvailabilityConf {
.version("1.4.0")
.stringConf
.createOptional
+
+ val HA_ZK_ENGINE_POOL_SIZE: ConfigEntry[Int] =
+ buildConf("ha.engine.pool.size")
+ .doc("Maximum number of engines when using USER share level.")
+ .version("1.4.0")
+ .intConf
+ .checkValue(s => s > 0 && s < 50, "Invalid Engine Pool Size.")
+ .createWithDefault(1)
+
+ val HA_ZK_ENGINE_PROVIDE_POLICY: ConfigEntry[String] =
+ buildConf("ha.engine.provide.policy")
+ .doc("Multiple engines can be exposed under same space when using USER share level, " +
+ "choose the appropriate strategy according to different scenarios.")
+ .version("1.4.0")
+ .stringConf
+ .createWithDefault(ProvidePolicy.RANDOM.toString)
Review comment:
```
.stringConf
.transform(_.toUpperCase(Locale.ROOT))
.checkValues(ProvidePolicy.values.map(_.toString))
```
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693732271
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
+
+ val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ def getEngineByPolicy(
+ zkClient: CuratorFramework,
+ namespace: String,
+ providePolicy: ProvidePolicy): Option[(String, Int)] = {
+ providePolicy match {
+ case RANDOM =>
+ Random.shuffle(getServiceNodesInfo(zkClient, namespace, silent = true)) match {
Review comment:
@zwangsheng, you're right, had improved the logic.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (25c332b) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.59%`.
> The diff coverage is `47.05%`.
> :exclamation: Current head 25c332b differs from pull request most recent head f5b4349. Consider uploading reports for the commit f5b4349 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.48% +0.59%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5431 -24
Branches 649 644 -5
============================================
+ Hits 4358 4371 +13
+ Misses 717 680 -37
Partials 380 380
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `47.50% <0.00%> (+1.15%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.14% <91.66%> (-1.14%)` | :arrow_down: |
| [...-common/src/main/scala-2.12/scala/util/Using.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS0yLjEyL3NjYWxhL3V0aWwvVXNpbmcuc2NhbGE=) | | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...f5b4349](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f5b4349) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.59%`.
> The diff coverage is `47.05%`.
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.48% +0.59%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5431 -24
Branches 649 644 -5
============================================
+ Hits 4358 4371 +13
+ Misses 717 680 -37
Partials 380 380
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `21.21% <0.00%> (-32.64%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `47.50% <0.00%> (+1.15%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `76.92% <81.25%> (+0.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.14% <91.66%> (-1.14%)` | :arrow_down: |
| [...-common/src/main/scala-2.12/scala/util/Using.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS0yLjEyL3NjYWxhL3V0aWwvVXNpbmcuc2NhbGE=) | | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...f5b4349](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3058654) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.57%`.
> The diff coverage is `47.05%`.
> :exclamation: Current head 3058654 differs from pull request most recent head 1e456f4. Consider uploading reports for the commit 1e456f4 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.46% +0.57%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 644 -5
============================================
+ Hits 4358 4369 +11
+ Misses 717 682 -35
+ Partials 380 379 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `46.66% <0.00%> (+0.32%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0Fic3RyYWN0U2Vzc2lvbi5zY2FsYQ==) | `95.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...1e456f4](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3058654) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.57%`.
> The diff coverage is `47.05%`.
> :exclamation: Current head 3058654 differs from pull request most recent head 2ff39b9. Consider uploading reports for the commit 2ff39b9 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.46% +0.57%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 644 -5
============================================
+ Hits 4358 4369 +11
+ Misses 717 682 -35
+ Partials 380 379 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `46.66% <0.00%> (+0.32%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0Fic3RyYWN0U2Vzc2lvbi5zY2FsYQ==) | `95.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...2ff39b9](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693826303
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
EngineServiceDiscovery still visited at the engine side in this patch.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] zwangsheng commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
zwangsheng commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693652318
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
##########
@@ -108,4 +109,19 @@ object HighAvailabilityConf {
.version("1.4.0")
.stringConf
.createOptional
+
+ val HA_ZK_ENGINE_POOL_SIZE: ConfigEntry[Int] =
+ buildConf("ha.engine.pool.size")
+ .doc("Maximum number of engines when using USER share level.")
+ .version("1.4.0")
+ .intConf
+ .createWithDefault(1)
Review comment:
How about use `checkValues` to make sure this configuration remains positive?
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693911573
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
Thanks for raising concerns:
> it always needs to create a new engine even there is an available engine there
This is the characteristic of the auto-mechanism, we can provide option to skip create a new engine.
> if we can fall back to reusing after failures in 2), the time cost is inevitable.
This work as `subDomain`. When using `subDomain` and encountering failure, time-consuming is also unavoidable.
**Improve**
- provide option to skip create a new engine
- fall back to reusing after creating failure
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694165553
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
Not exactly too, you add a zookeeper distribute lock for all connections, this is much heavier than 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694224424
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
+
+ val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ def getEngineByPolicy(
+ zkClient: CuratorFramework,
+ namespace: String,
+ providePolicy: ProvidePolicy): Option[(String, Int)] = {
+ providePolicy match {
+ case RANDOM =>
+ Random.shuffle(getServiceNodesInfo(zkClient, namespace, silent = true)).take(1) match {
+ case Seq(sn) => Some((sn.host, sn.port))
+ case _ => None
+ }
+ case _ => throw new IllegalArgumentException(s"Not support provide policy $providePolicy")
Review comment:
Seems not required to add a default branch for enum, pls remove it and check if the compiler print any warnings
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694169817
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerUserSuite.scala
##########
@@ -30,29 +33,118 @@ class KyuubiOperationPerUserSuite extends WithKyuubiServer with JDBCTests {
KyuubiConf().set(KyuubiConf.ENGINE_SHARE_LEVEL, "user")
}
- test("ensure two connections in user mode share the same engine") {
- var r1: String = null
- var r2: String = null
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r1 = res.getString("value")
+ test("ensure two connections share the same engine when engine pool size is 1.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "1",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "aaa"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r2 = res.getString("value")
+ assert(r1 === r2)
+ }
+ }
+
+ test("ensure two connections don't share the same engine when engine pool is 2.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "2",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "bbb",
+ KyuubiConf.ENGINE_IDLE_TIMEOUT.key -> "30000"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- eventually(timeout(120.seconds), interval(100.milliseconds)) {
- assert(r1 != null && r2 != null)
+ assert(r1 != r2)
}
+ }
- assert(r1 === r2)
+ test("ensure two of three connections share the same engine when engine pool size is 2.") {
Review comment:
Let me have a try.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693827686
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694210752
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
##########
@@ -108,4 +109,20 @@ object HighAvailabilityConf {
.version("1.4.0")
.stringConf
.createOptional
+
+ val HA_ZK_ENGINE_POOL_SIZE: ConfigEntry[Int] =
+ buildConf("ha.engine.pool.size")
+ .doc("Maximum number of engines when using USER share level.")
+ .version("1.4.0")
+ .intConf
+ .checkValue(s => s > 0 && s < 50, "Invalid Engine Pool Size.")
Review comment:
`"Invalid Engine Pool Size."` => `"Invalid Engine Pool Size, should be between 0 and 50"`
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694208548
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
:sweat_smile: Maybe I am not clear-headed now, will back here tomorrow.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694179274
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
we are talking about the engine pool itself, for what I am trying to improve based on your current implementation. I am not sure if my concerns can be ignored or persuade like that.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693710296
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
Updated `KyuubiOperationPerUserSuite`, I am not sure if the coding is elegant.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694224424
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
+
+ val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ def getEngineByPolicy(
+ zkClient: CuratorFramework,
+ namespace: String,
+ providePolicy: ProvidePolicy): Option[(String, Int)] = {
+ providePolicy match {
+ case RANDOM =>
+ Random.shuffle(getServiceNodesInfo(zkClient, namespace, silent = true)).take(1) match {
+ case Seq(sn) => Some((sn.host, sn.port))
+ case _ => None
+ }
+ case _ => throw new IllegalArgumentException(s"Not support provide policy $providePolicy")
Review comment:
Seems not required to add a default branch for enum, pls remove it and see if the compiler print any warnings
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694190595
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
Now the class EngineServiceDiscovery is for the engine module to visit AS-IS, but the new object EngineServiceDiscovery is used by the server module
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693808400
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -125,12 +128,34 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
}
- private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ def getOrCreate(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
Review comment:
Hmm, we need to wait for the lock every time..
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 closed pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 closed pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694069373
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
> This is the characteristic of the auto-mechanism, we can provide option to skip create a new engine.
It makes the feature itself even more unfriendly.
> This work as subDomain. When using subDomain and encountering failure, time-consuming is also unavoidable.
Not really the same. the original intention of a subdomain is to provide isolation but creating multiple engines under a subdomain is for speed and power. But now, it does bring extra creation overhead for a lot of cases
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903831033
> I'd like to ask if we have the configuration of engine expiration recycling?
Hi, my understanding is that there is no such configuration currently, we can clear it through kyuubi-ctl, hope answered your question.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-904520407
#### 一些想法
##### 什么是引擎池？
一个引擎池是面向一个租户的，不同的用户使用不同的引擎池。一个引擎池是在一个目录下或者根据相应的目录过滤条件可发现的引擎集合。
假设我们为tom和jerry两位用户开启了引擎池。前者的大小为2，后者的大小为3。
所以，tom的引擎池看起来是这样的
```
kyuubi_USER/tom/engine-pool-0/serviceUri=ip:port;version=1.3.0;sequence=..../data
kyuubi_USER/tom/engine-pool-1/serviceUri=ip:port;version=1.3.0;sequence=..../data
```
而jerry的引擎池则看起来是这样的
```
kyuubi_USER/jerry/engine-pool-0/serviceUri=ip:port;version=1.3.0;sequence=..../data
kyuubi_USER/jerry/engine-pool-1/serviceUri=ip:port;version=1.3.0;sequence=..../data
kyuubi_USER/jerry/engine-pool-2/serviceUri=ip:port;version=1.3.0;sequence=..../data
```
注意，这里的整个“目录”设计规则和Kyuubi目前的规则没有任何的区别。其中，
- 第一层目录为server space加下划线再加share level，这里的share_level会是USER/Server。
- 第二层目录为用户名
- 第三层目录为subDomain。这里使我们需要关注的重点，因为引擎池化的逻辑主要围绕这一层目录的查找、创建暂开。我们可以复用和拓展现有的并发控制逻辑。在我们给的例子中’engine-pool-‘没有特别的含义，我们后面可以将它设置为参数或者hardcode的字符串前缀。而后缀则是0到pool size -1的自然数序列。如上面的例子所示，tom的引擎池会产生2个subDomain的目录。
- 剩余的目录就和现在的设计一样了，需强调的是和以前一样一个目录下依然只维护一个engine实例
##### 引擎池大小控制
- `kyuubi.engine.pool.size.threshold`, 引入该参数为服务端参数，由服务端控制引擎池的绝对上限。如果用户的设置超过这个设置值，则报错或者取该值
- `kyuubi.engine.pool.size`，引入该参数为用户和服务端都可设置的参数，参数取值范围在`kyuubi.engine.pool.size.threshold`以下。当服务端统一或者用用户默认参数设置该值时，服务端会在统计当前存在subDomain目录数，即当前池大小，如果还未达预期，则递增一个subDomain目录并在该目录下创建一个引擎。
引擎的生命周期和现在的实现不变，可依据现在的缓存方式进行自我缓存，当生命周期到达上限，则自己退出并删除自己所属的subDomain
##### 引擎的创建和复用 (RANDOM)
在 RANDOM 模式下，一切都是随机的，引擎的复用和引擎的创建在这个 POLICY 下是等价的。
举例，当jerry 在随机算法下使用连接池，假设 engine pool 的中间值为如下所示的状态，槽0和2已经有 engine 实例，而槽1 还未被初始化
```
kyuubi_USER/jerry/engine-pool-0/serviceUri=ip:port;version=1.3.0;sequence=..../data
kyuubi_USER/jerry/engine-pool-1/
kyuubi_USER/jerry/engine-pool-2/serviceUri=ip:port;version=1.3.0;sequence=..../data
```
此时，当 jerry 尝试连接时，
- jerry 显示配置sub.domain，几个特例
- sub.domain=engine-pool-0, 复用连接池内槽0的实例
- sub.domain=engine-pool-1，**只需对 engine-pool-1 加锁**，直接创建槽1的实例，“手动”完成初始化，并连接
- sub.domain=someothers，绕过引擎池的使用
- jerry 未配置sub.domain， 则随机算法生成 engine-pool-x
- 当x=0时，复用连接池内槽0的实例
- 当x=1时，只需对 engine-pool-1 加锁，创建槽1的实例，完成初始化
- 当x=2时，复用连接池内槽1的实例
- jerry 使用其他模式，照旧
##### 两个 Kyuubi Server 对某用户配置了不同的 kyuubi.engine.pool.size 值怎么办？
假如 Server1 为4，Server2 为 5，则Server1，在[0, 3]区间内随机，而Server2可在[0, 4]之间取值。两者不存在耦合关系，当然最好的实践还是配置相同的参数值
##### kyuubi.engine.pool.size 是否对用户暴露，暴露后用户连接每次 kyuubi.engine.pool.size 不一致怎么办？不合理设置怎么办？
理论上可以对用户直接暴露，
当用户设置不合理时，比如过大，则由kyuubi.engine.pool.size.threshold 控制
当用户设置不一致时，比如增加，则 pool size 可随机增加。抑或减小，则新区间外的引擎将因随机不到而自我回收。
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] zwangsheng commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
zwangsheng commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693708918
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
+
+ val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ def getEngineByPolicy(
+ zkClient: CuratorFramework,
+ namespace: String,
+ providePolicy: ProvidePolicy): Option[(String, Int)] = {
+ providePolicy match {
+ case RANDOM =>
+ Random.shuffle(getServiceNodesInfo(zkClient, namespace, silent = true)) match {
Review comment:
I have a question, please correct me if there is any problem.Here All engines under namespce are obtained. If there is one engine, the create phase is not entered, and the engine state is always 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693787153
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerUserSuite.scala
##########
@@ -30,29 +31,68 @@ class KyuubiOperationPerUserSuite extends WithKyuubiServer with JDBCTests {
KyuubiConf().set(KyuubiConf.ENGINE_SHARE_LEVEL, "user")
}
- test("ensure two connections in user mode share the same engine") {
- var r1: String = null
- var r2: String = null
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r1 = res.getString("value")
- }
- }.start()
+ test("ensure two connections share the same engine when engine pool size is 1.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "1",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "aaa"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r2 = res.getString("value")
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- eventually(timeout(120.seconds), interval(100.milliseconds)) {
- assert(r1 != null && r2 != null)
+ assert(r1 === r2)
}
+ }
+
+ test("ensure two connections don't share the same engine when engine pool size bigger than 1.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "2",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "bbb"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
- assert(r1 === r2)
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
+ }
+
+ assert(r1 != r2)
Review comment:
can we add a test for connect 3？
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694167302
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerUserSuite.scala
##########
@@ -30,29 +33,118 @@ class KyuubiOperationPerUserSuite extends WithKyuubiServer with JDBCTests {
KyuubiConf().set(KyuubiConf.ENGINE_SHARE_LEVEL, "user")
}
- test("ensure two connections in user mode share the same engine") {
- var r1: String = null
- var r2: String = null
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r1 = res.getString("value")
+ test("ensure two connections share the same engine when engine pool size is 1.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "1",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "aaa"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r2 = res.getString("value")
+ assert(r1 === r2)
+ }
+ }
+
+ test("ensure two connections don't share the same engine when engine pool is 2.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "2",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "bbb",
+ KyuubiConf.ENGINE_IDLE_TIMEOUT.key -> "30000"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- eventually(timeout(120.seconds), interval(100.milliseconds)) {
- assert(r1 != null && r2 != null)
+ assert(r1 != r2)
}
+ }
- assert(r1 === r2)
+ test("ensure two of three connections share the same engine when engine pool size is 2.") {
Review comment:
Can we add another case that connection 2, 3 concurrently created while connection 1's engine creation is finished
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903958090
> > Hi, my understanding is that there is no such configuration currently, we can clear it through kyuubi-ctl, hope answered your question.
>
> I agree that the idle engine can be cleared through Kyuubi cli, but considering that Kyuubi is a resident service, maybe the PR can mention relevant parameters later?
ok, will find a suitable location to mention relevant parameters.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6aad2cc) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.58%`.
> The diff coverage is `47.05%`.
> :exclamation: Current head 6aad2cc differs from pull request most recent head 3f2ed9e. Consider uploading reports for the commit 3f2ed9e to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.87% 80.46% +0.58%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 644 -5
============================================
+ Hits 4357 4369 +12
+ Misses 719 681 -38
- Partials 379 380 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `47.50% <0.00%> (+1.97%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0Fic3RyYWN0U2Vzc2lvbi5zY2FsYQ==) | `95.50% <100.00%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `47.36% <0.00%> (-2.64%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...3f2ed9e](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694181652
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
the logging is not used anywhere too
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-904520407
#### 一些想法
##### 什么是引擎池？
一个引擎池是面向一个租户的，不同的用户使用不同的引擎池。一个引擎池是在一个目录下或者根据相应的目录过滤条件可发现的引擎集合。
假设我们为tom和jerry两位用户开启了引擎池。前者的大小为2，后者的大小为3。
所以，tom的引擎池看起来是这样的
```
kyuubi_USER/tom/engine-pool-0/serviceUri=ip:port;version=1.3.0;sequence=..../data
kyuubi_USER/tom/engine-pool-1/serviceUri=ip:port;version=1.3.0;sequence=..../data
```
而jerry的引擎池则看起来是这样的
```
kyuubi_USER/jerry/engine-pool-0/serviceUri=ip:port;version=1.3.0;sequence=..../data
kyuubi_USER/jerry/engine-pool-1/serviceUri=ip:port;version=1.3.0;sequence=..../data
kyuubi_USER/jerry/engine-pool-2/serviceUri=ip:port;version=1.3.0;sequence=..../data
```
注意，这里的整个“目录”设计规则和Kyuubi目前的规则没有任何的区别。其中，
- 第一层目录为server space加下划线再加share level，这里的share_level会是USER/Server。
- 第二层目录为用户名
- 第三层目录为subDomain。这里使我们需要关注的重点，因为引擎池化的逻辑主要围绕这一层目录的查找、创建暂开。我们可以复用和拓展现有的并发控制逻辑。在我们给的例子中’engine-pool-‘没有特别的含义，我们后面可以将它设置为参数或者hardcode的字符串前缀。而后缀则是0到pool size -1的自然数序列。如上面的例子所示，tom的引擎池会产生2个subDomain的目录。
- 剩余的目录就和现在的设计一样了，需强调的是和以前一样一个目录下依然只维护一个engine实例
##### 引擎池大小控制
- `kyuubi.engine.pool.size.threshold`, 引入该参数为服务端参数，由服务端控制引擎池的绝对上限。如果用户的设置超过这个设置值，则报错或者取该值
- `kyuubi.engine.pool.size`，引入该参数为用户和服务端都可设置的参数，参数取值范围在`kyuubi.engine.pool.size.threshold`以下。当服务端统一或者用用户默认参数设置该值时，服务端会在统计当前存在subDomain目录数，即当前池大小，如果还未达预期，则递增一个subDomain目录并在该目录下创建一个引擎。
引擎的生命周期和现在的实现不变，可依据现在的缓存方式进行自我缓存，当生命周期到达上限，则自己退出并删除自己所属的subDomain
##### 引擎的创建和复用 (RANDOM)
在 RANDOM 模式下，一切都是随机的，引擎的复用和引擎的创建在这个 POLICY 下是等价的。
举例，当jerry 在随机算法下使用连接池，假设 engine pool 的中间值为如下所示的状态，槽0和2已经有 engine 实例，而槽1 还未被初始化
```
kyuubi_USER/jerry/engine-pool-0/serviceUri=ip:port;version=1.3.0;sequence=..../data
kyuubi_USER/jerry/engine-pool-1/
kyuubi_USER/jerry/engine-pool-2/serviceUri=ip:port;version=1.3.0;sequence=..../data
```
此时，当 jerry 尝试连接时，
- jerry 显示配置sub.domain，几个特例
- sub.domain=engine-pool-0, 复用连接池内槽0的实例
- sub.domain=engine-pool-1，**只需对 engine-pool-1 加锁**，直接创建槽1的实例，“手动”完成初始化，并连接
- sub.domain=someothers，绕过引擎池的使用
- jerry 未配置sub.domain， 则随机算法生成 engine-pool-x
- 当x=0时，复用连接池内槽0的实例
- 当x=1时，只需对 engine-pool-1 加锁，创建槽1的实例，完成初始化
- 当x=2时，复用连接池内槽1的实例
- jerry 使用其他模式，照旧
##### 两个 Kyuubi Server 对某用户配置了不同的 kyuubi.engine.pool.size 值怎么办？
假如 Server1 为4，Server2 为 5，则Server1，在[0, 3]区间内随机，而Server2可在[0, 4]之间取值。两者不存在耦合关系，当然最好的实践还是配置相同的参数值
##### kyuubi.engine.pool.size 是否对用户暴露，暴露后用户连接每次 kyuubi.engine.pool.size 不一致怎么办？不合理设置怎么办？
理论上可以对用户直接暴露，
当用户设置不合理时，比如过大，则由kyuubi.engine.pool.size.threshold 控制
当用户设置不一致时，比如增加，则 pool size 可随机增加。抑或减小，则新区间外的引擎将因随机不到而自我回收。
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693708561
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerUserSuite.scala
##########
@@ -21,13 +21,15 @@ import org.scalatest.time.SpanSugar._
import org.apache.kyuubi.WithKyuubiServer
import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.ha.HighAvailabilityConf
class KyuubiOperationPerUserSuite extends WithKyuubiServer with JDBCTests {
override protected def jdbcUrl: String = getJdbcUrl
override protected val conf: KyuubiConf = {
KyuubiConf().set(KyuubiConf.ENGINE_SHARE_LEVEL, "user")
+ KyuubiConf().set(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE, 1)
Review comment:
Make sense, thanks.
Had updated the test case.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694216592
##########
File path: dev/dependencyList
##########
@@ -29,6 +29,8 @@ htrace-core4/4.1.0-incubating//htrace-core4-4.1.0-incubating.jar
jackson-annotations/2.11.4//jackson-annotations-2.11.4.jar
jackson-core/2.11.4//jackson-core-2.11.4.jar
jackson-databind/2.11.4//jackson-databind-2.11.4.jar
+jackson-module-paranamer/2.11.4//jackson-module-paranamer-2.11.4.jar
Review comment:
Thanks for reviewing, 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (74245bc) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.57%`.
> The diff coverage is `46.00%`.
> :exclamation: Current head 74245bc differs from pull request most recent head f4ddb2e. Consider uploading reports for the commit f4ddb2e to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.87% 80.44% +0.57%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5432 -23
Branches 649 644 -5
============================================
+ Hits 4357 4370 +13
+ Misses 719 682 -37
- Partials 379 380 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `46.66% <0.00%> (+1.13%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.21% <86.66%> (+1.09%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...f4ddb2e](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f5b4349) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.59%`.
> The diff coverage is `47.05%`.
> :exclamation: Current head f5b4349 differs from pull request most recent head dc6ef3b. Consider uploading reports for the commit dc6ef3b to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.48% +0.59%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5431 -24
Branches 649 644 -5
============================================
+ Hits 4358 4371 +13
+ Misses 717 680 -37
Partials 380 380
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `21.21% <0.00%> (-32.64%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `47.50% <0.00%> (+1.15%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `76.92% <81.25%> (+0.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.14% <91.66%> (-1.14%)` | :arrow_down: |
| [...-common/src/main/scala-2.12/scala/util/Using.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS0yLjEyL3NjYWxhL3V0aWwvVXNpbmcuc2NhbGE=) | | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...dc6ef3b](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694200900
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
The `EngineRef` exists in the server module, so `engine module` == `server module` from current codebase.
Please correct me if wrong, 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f90201f) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.57%`.
> The diff coverage is `47.05%`.
> :exclamation: Current head f90201f differs from pull request most recent head 0d338a4. Consider uploading reports for the commit 0d338a4 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.87% 80.44% +0.57%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5432 -23
Branches 649 644 -5
============================================
+ Hits 4357 4370 +13
+ Misses 719 681 -38
- Partials 379 381 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `47.50% <0.00%> (+1.97%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.21% <86.66%> (+1.09%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0Fic3RyYWN0U2Vzc2lvbi5zY2FsYQ==) | `95.50% <100.00%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpU2VydmVyLnNjYWxh) | `47.36% <0.00%> (-2.64%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...0d338a4](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693801523
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
EngineServiceDiscovery is only visited at the engine side 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] zwangsheng commented on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
zwangsheng commented on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903717283
I'd like to ask if we have the configuration of engine expiration recycling?
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (56b9c88) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **decrease** coverage by `0.24%`.
> The diff coverage is `46.93%`.
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
- Coverage 79.87% 79.62% -0.25%
Complexity 11 11
============================================
Files 147 148 +1
Lines 5455 5497 +42
Branches 649 656 +7
============================================
+ Hits 4357 4377 +20
- Misses 719 739 +20
- Partials 379 381 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `46.66% <0.00%> (+1.13%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.21% <86.66%> (+1.09%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `98.52% <100.00%> (+0.25%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...56b9c88](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d43a989) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.60%`.
> The diff coverage is `46.00%`.
> :exclamation: Current head d43a989 differs from pull request most recent head 3f2ed9e. Consider uploading reports for the commit 3f2ed9e to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.87% 80.47% +0.60%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 644 -5
============================================
+ Hits 4357 4370 +13
+ Misses 719 680 -39
- Partials 379 380 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `47.50% <0.00%> (+1.97%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...-common/src/main/scala-2.12/scala/util/Using.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS0yLjEyL3NjYWxhL3V0aWwvVXNpbmcuc2NhbGE=) | | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...3f2ed9e](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694179274
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
we are talking about the engine pool itself, for what I am trying to improve based on your current implementation. I am not sure if my concerns can be ignored or persuade like that.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] cfmcgrady commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
cfmcgrady commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693705088
##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/engine/ProvidePolicy.scala
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+package org.apache.kyuubi.engine
+
+object ProvidePolicy extends Enumeration {
+
+ type ProvidePolicy = Value
+
+ val
+ /** A random policy that pick engines randomly. */
+ RANDOM = Value
Review comment:
Only `RANDOM` policy support now?
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693897894
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
? Isn't EngineServiceDiscovery still visited at the engine side in this patch?
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694201950
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -125,12 +128,34 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
}
- private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ def getOrCreate(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
Review comment:
Yeah, I am thinking how to improve this.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3058654) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.57%`.
> The diff coverage is `47.05%`.
> :exclamation: Current head 3058654 differs from pull request most recent head b5a92e4. Consider uploading reports for the commit b5a92e4 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.89% 80.46% +0.57%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 644 -5
============================================
+ Hits 4358 4369 +11
+ Misses 717 682 -35
+ Partials 380 379 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `46.66% <0.00%> (+0.32%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0Fic3RyYWN0U2Vzc2lvbi5zY2FsYQ==) | `95.50% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...b5a92e4](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694623803
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
> EngineServiceDiscovery is only visited at the engine side before
Get your point. just like class `ServiceDiscovery` is visited in engine side and object `ServiceDiscovery` is visited in servier side before. Will follow you if you have a better suggestion.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694507657
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -125,12 +128,34 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
}
- private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ def getOrCreate(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
+
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
+ val notFull = checkEnginePoolCapacity(zkClient, engineSpace)
+ if (notFull) {
+ createInternal(zkClient)
+ } else {
+ val engineRef = getEngineByPolicy(zkClient, engineSpace, providePolicy)
+ if (engineRef.nonEmpty) return engineRef.get
Review comment:
👍
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694208534
##########
File path: dev/dependencyList
##########
@@ -29,6 +29,8 @@ htrace-core4/4.1.0-incubating//htrace-core4-4.1.0-incubating.jar
jackson-annotations/2.11.4//jackson-annotations-2.11.4.jar
jackson-core/2.11.4//jackson-core-2.11.4.jar
jackson-databind/2.11.4//jackson-databind-2.11.4.jar
+jackson-module-paranamer/2.11.4//jackson-module-paranamer-2.11.4.jar
Review comment:
We need to update LICENSE-binary once `dependencyList` changed
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter edited a comment on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903279447
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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 [#974](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d43a989) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/271ccc948fbbc340815eed9b8d224541fbce2d11?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (271ccc9) will **increase** coverage by `0.60%`.
> The diff coverage is `46.00%`.
> :exclamation: Current head d43a989 differs from pull request most recent head 7afc58c. Consider uploading reports for the commit 7afc58c to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #974 +/- ##
============================================
+ Coverage 79.87% 80.47% +0.60%
Complexity 11 11
============================================
Files 147 147
Lines 5455 5430 -25
Branches 649 644 -5
============================================
+ Hits 4357 4370 +13
+ Misses 719 680 -39
- Partials 379 380 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...scala/org/apache/kyuubi/engine/ProvidePolicy.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvdmlkZVBvbGljeS5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...ache/kyuubi/ha/client/EngineServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9FbmdpbmVTZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `20.58% <0.00%> (-33.26%)` | :arrow_down: |
| [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `47.50% <0.00%> (+1.97%)` | :arrow_up: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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) | `77.92% <86.66%> (+1.80%)` | :arrow_up: |
| [...la/org/apache/kyuubi/ha/HighAvailabilityConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL0hpZ2hBdmFpbGFiaWxpdHlDb25mLnNjYWxh) | `97.10% <90.90%> (-1.18%)` | :arrow_down: |
| [...-common/src/main/scala-2.12/scala/util/Using.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/974/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS0yLjEyL3NjYWxhL3V0aWwvVXNpbmcuc2NhbGE=) | | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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/974?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 [271ccc9...7afc58c](https://codecov.io/gh/apache/incubator-kyuubi/pull/974?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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693678552
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/HighAvailabilityConf.scala
##########
@@ -108,4 +109,19 @@ object HighAvailabilityConf {
.version("1.4.0")
.stringConf
.createOptional
+
+ val HA_ZK_ENGINE_POOL_SIZE: ConfigEntry[Int] =
+ buildConf("ha.engine.pool.size")
+ .doc("Maximum number of engines when using USER share level.")
+ .version("1.4.0")
+ .intConf
+ .createWithDefault(1)
Review comment:
Make sense, 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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-904775487
@yaooqinn 优秀，思路清奇 👍，尝试实现一把。
同时，多谢各位的REVIEW、点评，收获很多的建议，包括逻辑、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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694181652
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
the logging is not used anywhere
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694222947
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
+
+ val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ def getEngineByPolicy(
+ zkClient: CuratorFramework,
+ namespace: String,
+ providePolicy: ProvidePolicy): Option[(String, Int)] = {
+ providePolicy match {
+ case RANDOM =>
+ Random.shuffle(getServiceNodesInfo(zkClient, namespace, silent = true)).take(1) match {
Review comment:
It can be simplified by `Seq.headOption`
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693711948
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
+
+ val mapper: ObjectMapper = new ObjectMapper().registerModule(DefaultScalaModule)
+
+ def getEngineByPolicy(
+ zkClient: CuratorFramework,
+ namespace: String,
+ providePolicy: ProvidePolicy): Option[(String, Int)] = {
+ providePolicy match {
+ case RANDOM =>
+ Random.shuffle(getServiceNodesInfo(zkClient, namespace, silent = true)) match {
Review comment:
Just wait a moment, try to understand your comment.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694172287
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerUserSuite.scala
##########
@@ -30,29 +33,118 @@ class KyuubiOperationPerUserSuite extends WithKyuubiServer with JDBCTests {
KyuubiConf().set(KyuubiConf.ENGINE_SHARE_LEVEL, "user")
}
- test("ensure two connections in user mode share the same engine") {
- var r1: String = null
- var r2: String = null
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r1 = res.getString("value")
+ test("ensure two connections share the same engine when engine pool size is 1.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "1",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "aaa"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- new Thread {
- override def run(): Unit = withJdbcStatement() { statement =>
- val res = statement.executeQuery("set spark.app.name")
- assert(res.next())
- r2 = res.getString("value")
+ assert(r1 === r2)
+ }
+ }
+
+ test("ensure two connections don't share the same engine when engine pool is 2.") {
+ withSessionConf()(
+ Map(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE.key -> "2",
+ KyuubiConf.ENGINE_SHARE_LEVEL_SUB_DOMAIN.key -> "bbb",
+ KyuubiConf.ENGINE_IDLE_TIMEOUT.key -> "30000"
+ ))(Map.empty) {
+
+ var r1: String = null
+ var r2: String = null
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r1 = res.getString("value")
+ }
+ }.start()
+
+ new Thread {
+ override def run(): Unit = withJdbcStatement() { statement =>
+ val res = statement.executeQuery("set spark.app.name")
+ assert(res.next())
+ r2 = res.getString("value")
+ }
+ }.start()
+
+ eventually(timeout(120.seconds), interval(100.milliseconds)) {
+ assert(r1 != null && r2 != null)
}
- }.start()
- eventually(timeout(120.seconds), interval(100.milliseconds)) {
- assert(r1 != null && r2 != null)
+ assert(r1 != r2)
}
+ }
- assert(r1 === r2)
+ test("ensure two of three connections share the same engine when engine pool size is 2.") {
Review comment:
> Can we add another case that connection 2, 3 concurrently created while connection 1's engine creation is finished
hi @yaooqinn, what's the aim of the test case? what will we assert?
like following?
```
assert(r1 == r2 || r1 === r3)
```
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] timothy65535 commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693687430
##########
File path: kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala
##########
@@ -126,11 +130,33 @@ private[kyuubi] class EngineRef private(conf: KyuubiConf, user: String, sessionI
}
private def create(zkClient: CuratorFramework): (String, Int) = tryWithLock(zkClient) {
- // TODO: improve this after support engine pool. (KYUUBI #918)
- var engineRef = getServerHost(zkClient, engineSpace)
- // Get the engine address ahead if another process has succeeded
- if (engineRef.nonEmpty) return engineRef.get
+ // USER share level support engine pool
+ if (shareLevel.equals(USER)) {
+
+ // create engine space
+ createEngineSpaceIfNotExists(zkClient, engineSpace, conf.get(HA_ZK_ENGINE_POOL_SIZE))
+
+ // create engine if not reach pool size
Review comment:
When creating engine, will check `pool.size` and `engineNum`.
_`pool size`_: read from engine space.
_`engine num`_: children of engine space.
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] zwangsheng commented on pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
zwangsheng commented on pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#issuecomment-903864114
> Hi, my understanding is that there is no such configuration currently, we can clear it through kyuubi-ctl, hope answered your question.
I agree that the idle engine can be cleared through Kyuubi cli, but considering that Kyuubi is a resident service, maybe the PR can mention relevant parameters later?
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] cfmcgrady commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
cfmcgrady commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r693706417
##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/operation/KyuubiOperationPerUserSuite.scala
##########
@@ -21,13 +21,15 @@ import org.scalatest.time.SpanSugar._
import org.apache.kyuubi.WithKyuubiServer
import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.ha.HighAvailabilityConf
class KyuubiOperationPerUserSuite extends WithKyuubiServer with JDBCTests {
override protected def jdbcUrl: String = getJdbcUrl
override protected val conf: KyuubiConf = {
KyuubiConf().set(KyuubiConf.ENGINE_SHARE_LEVEL, "user")
+ KyuubiConf().set(HighAvailabilityConf.HA_ZK_ENGINE_POOL_SIZE, 1)
Review comment:
It's better to add more test cases for `HA_ZK_ENGINE_POOL_SIZE > 1`?
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #974: [KYUUBI #962] Support multiple engines under USER share level
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #974:
URL: https://github.com/apache/incubator-kyuubi/pull/974#discussion_r694204222
##########
File path: kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/EngineServiceDiscovery.scala
##########
@@ -52,3 +61,59 @@ class EngineServiceDiscovery private(
super.stop()
}
}
+
+object EngineServiceDiscovery extends Logging {
Review comment:
I mean module like
 not a package
--
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: commits-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org