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/09 06:27:17 UTC
[GitHub] [incubator-kyuubi] timothy65535 opened a new pull request #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
timothy65535 opened a new pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/contributions.html
2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
### 1. Describe the feature
Kyuubi use `TestingServer` as the EmbeddedZookeeper service in codebase, and require many args to init the service. These `electionPort`, `quorumPort`, `serverId` parameters only work in cluster mode, these ports are not bound to the host.
From hadoop and bookkeeper project, they use `ZooKeeperServer` as a built-in zk service, especially the bookeeper project.
- https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-registry/src/main/java/org/apache/hadoop/registry/server/services/MicroZookeeperService.java
- https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/shims/zk/ZooKeeperServerShimImpl.java
### 2. Describe the solution
Improve
- Remove `electionPort`, `quorumPort`, `serverId` which are nerver be used, hadoop and bookkeer do the same
- Replace `TestingServer` with `ZooKeeperServer`
- Remove `curator-test` dependency
### 3. `deleteDataDirectoryOnClose`
The name style for `deleteDataDirectoryOnClose` option, refer to hadoop `core-default.xml`
https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/core-default.xml
```
hadoop.security.dns.log-slow-lookups.enabled
hadoop.security.dns.log-slow-lookups.threshold.ms
dfs.client.block.write.replace-datanode-on-failure.policy
```
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] [Run test](https://kyuubi.readthedocs.io/en/latest/tools/testing.html#running-tests) locally before make a pull request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-896443021
> thanks, merging to master
Thanks @ulysses-you
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-896006151
I mean you need send a PR according to the doc I attached
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-896005013
Travis CI queued again.
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685861237
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -56,12 +56,14 @@ object ZookeeperConf {
.version("1.2.0")
.fallbackConf(EMBEDDED_ZK_TEMP_DIR)
+ @deprecated(s"the config will be removed", since = "1.3.0")
Review comment:
Yeah, they don't print any warning or blocks anything after being removed. We keep these as @pan3793 suggested.
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895740904
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 [#907](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ce06a0f) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/89d031264b0332aced5a8c104cd14789810cbdea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89d0312) will **decrease** coverage by `0.04%`.
> The diff coverage is `75.00%`.
> :exclamation: Current head ce06a0f differs from pull request most recent head 7d7fde9. Consider uploading reports for the commit 7d7fde9 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907 +/- ##
============================================
- Coverage 79.18% 79.14% -0.05%
Complexity 11 11
============================================
Files 143 145 +2
Lines 5391 5418 +27
Branches 652 656 +4
============================================
+ Hits 4269 4288 +19
- Misses 757 760 +3
- Partials 365 370 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ommon/src/main/scala/org/apache/kyuubi/Utils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9VdGlscy5zY2FsYQ==) | `70.23% <0.00%> (-3.18%)` | :arrow_down: |
| [...rg/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvRW1iZWRkZWRab29rZWVwZXIuc2NhbGE=) | `86.66% <85.00%> (-3.58%)` | :arrow_down: |
| [...la/org/apache/kyuubi/zookeeper/ZookeeperConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvWm9va2VlcGVyQ29uZi5zY2FsYQ==) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9TcGFya1NRTEVuZ2luZS5zY2FsYQ==) | `69.69% <0.00%> (-4.28%)` | :arrow_down: |
| [.../engine/spark/session/SparkSQLSessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9zZXNzaW9uL1NwYXJrU1FMU2Vzc2lvbk1hbmFnZXIuc2NhbGE=) | `91.66% <0.00%> (-3.08%)` | :arrow_down: |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `58.49% <0.00%> (-1.51%)` | :arrow_down: |
| [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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=) | `79.41% <0.00%> (-0.99%)` | :arrow_down: |
| [...pache/kyuubi/engine/spark/events/EngineEvent.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvRW5naW5lRXZlbnQuc2NhbGE=) | `89.65% <0.00%> (-0.35%)` | :arrow_down: |
| [...kyuubi/engine/spark/session/SparkSessionImpl.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9zZXNzaW9uL1NwYXJrU2Vzc2lvbkltcGwuc2NhbGE=) | `100.00% <0.00%> (ø)` | |
| ... and [9 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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/907?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 [89d0312...7d7fde9](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r684966249
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -87,26 +75,26 @@ object ZookeeperConf {
.intConf
.createWithDefault(120)
- val ZK_MIN_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MIN_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.min.session.timeout")
.doc("minSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 2 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 2)
- val ZK_MAX_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MAX_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.max.session.timeout")
.doc("maxSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 20 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 20)
- val ZK_SERVER_ID: ConfigEntry[Int] =
Review comment:
This is a breaking change, let's mark it deprecated and useless first, then remove it after v1.3.0 release.
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r684966249
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -87,26 +75,26 @@ object ZookeeperConf {
.intConf
.createWithDefault(120)
- val ZK_MIN_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MIN_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.min.session.timeout")
.doc("minSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 2 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 2)
- val ZK_MAX_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MAX_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.max.session.timeout")
.doc("maxSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 20 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 20)
- val ZK_SERVER_ID: ConfigEntry[Int] =
Review comment:
This is a breaking change, let mark it deprecated and useless first, then remove it after v1.3.0 release.
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -87,26 +75,26 @@ object ZookeeperConf {
.intConf
.createWithDefault(120)
- val ZK_MIN_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MIN_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.min.session.timeout")
.doc("minSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 2 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 2)
- val ZK_MAX_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MAX_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.max.session.timeout")
.doc("maxSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 20 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 20)
- val ZK_SERVER_ID: ConfigEntry[Int] =
- buildConf("zookeeper.embedded.server.id")
- .doc("serverId for the embedded zookeeper server")
+ val ZK_DELETE_DATA_DIRECTORY_ON_CLOSE: ConfigEntry[Boolean] =
+ buildConf("zookeeper.embedded.delete-data-directory-on-close")
+ .doc("delete the storage directory of embedded zookeeper when service close")
.version("1.2.0")
Review comment:
1.3.0
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685870248
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -56,12 +56,14 @@ object ZookeeperConf {
.version("1.2.0")
.fallbackConf(EMBEDDED_ZK_TEMP_DIR)
+ @deprecated(s"the config will be removed", since = "1.3.0")
Review comment:
If we choose to keep this, we need to notify users at least; otherwise, they are completely dead code
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685042350
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala
##########
@@ -17,77 +17,83 @@
package org.apache.kyuubi.zookeeper
-import java.io.File
-import java.net.InetAddress
+import java.io.{File, IOException}
+import java.net.{InetAddress, InetSocketAddress, ServerSocket}
-import scala.collection.JavaConverters._
-
-import org.apache.curator.test.{InstanceSpec, TestingServer}
+import org.apache.zookeeper.server.{NIOServerCnxnFactory, ZooKeeperServer}
import org.apache.kyuubi.Utils._
import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.service.{AbstractService, ServiceState}
import org.apache.kyuubi.zookeeper.ZookeeperConf._
class EmbeddedZookeeper extends AbstractService("EmbeddedZookeeper") {
- private var spec: InstanceSpec = _
- private var server: TestingServer = _
- // When the client port is 0, the TestingServer will not randomly pick free local port to use
- // So adjust it to -1 to achieve what is common cognition.
- private def normalizePort(port: Int): Int = if (port == 0) -1 else port
+ private var zks: ZooKeeperServer = _
+ private var serverFactory: NIOServerCnxnFactory = _
+ private var dataDirectory: File = _
+ private var deleteDataDirectoryOnClose: Boolean = _
override def initialize(conf: KyuubiConf): Unit = synchronized {
- val dataDirectory = new File(conf.get(ZK_DATA_DIR))
+ dataDirectory = new File(conf.get(ZK_DATA_DIR))
+ deleteDataDirectoryOnClose = conf.get(ZK_DELETE_DATA_DIRECTORY_ON_CLOSE)
val clientPort = normalizePort(conf.get(ZK_CLIENT_PORT))
- val electionPort = normalizePort(conf.get(ZK_ELECTION_PORT))
- val quorumPort = normalizePort(conf.get(ZK_QUORUM_PORT))
- val serverId = conf.get(ZK_SERVER_ID)
val tickTime = conf.get(ZK_TICK_TIME)
val maxClientCnxns = conf.get(ZK_MAX_CLIENT_CONNECTIONS)
- // TODO: Is it right in prod?
- val deleteDataDirectoryOnClose = true
- val customProperties =
- (conf.get(ZK_MIN_SESSION_TIMEOUT).map { timeout =>
- "minSessionTimeout" -> Integer.valueOf(timeout)
- } ++ conf.get(ZK_MAX_SESSION_TIMEOUT).map { timeout =>
- "maxSessionTimeout" -> Integer.valueOf(timeout)
- }).toMap[String, Object].asJava
-
+ val minSessionTimeout = conf.get(ZK_MIN_SESSION_TIMEOUT)
+ val maxSessionTimeout = conf.get(ZK_MAX_SESSION_TIMEOUT)
val hostname = conf.get(ZK_CLIENT_PORT_ADDRESS).map(InetAddress.getByName)
.getOrElse(findLocalInetAddress).getCanonicalHostName
- spec = new InstanceSpec(
- dataDirectory,
- clientPort,
- electionPort,
- quorumPort,
- deleteDataDirectoryOnClose,
- serverId,
- tickTime,
- maxClientCnxns,
- customProperties,
- hostname)
- server = new TestingServer(spec, false)
+
+ zks = new ZooKeeperServer(dataDirectory, dataDirectory, tickTime)
+ zks.setMinSessionTimeout(minSessionTimeout)
+ zks.setMaxSessionTimeout(maxSessionTimeout)
+
+ serverFactory = new NIOServerCnxnFactory
+ serverFactory.configure(new InetSocketAddress(hostname, clientPort), maxClientCnxns)
+
super.initialize(conf)
}
override def start(): Unit = synchronized {
- server.start()
+ serverFactory.startup(zks)
info(s"$getName is started at $getConnectString")
// Stop the EmbeddedZookeeper after the Kyuubi server stopped
- addShutdownHook(() => server.close(), SERVER_SHUTDOWN_PRIORITY - 1)
+ addShutdownHook(() => stop(), SERVER_SHUTDOWN_PRIORITY - 1)
super.start()
}
override def stop(): Unit = synchronized {
if (getServiceState == ServiceState.STARTED) {
- server.close()
+ if (null != serverFactory) serverFactory.shutdown()
+ if (null != zks) zks.shutdown()
+ if (deleteDataDirectoryOnClose) deleteDirectoryRecursively(dataDirectory)
}
super.stop()
}
def getConnectString: String = synchronized {
- assert(spec != null, s"$getName is in $getServiceState")
- spec.getConnectString
+ assert(zks != null, s"$getName is in $getServiceState")
+ s"${serverFactory.getLocalAddress.getHostName}:${serverFactory.getLocalPort}"
+ }
+
+ private def normalizePort(port: Int): Int = if (port <= 0) getRandomAvailablePort else port
+
+ /**
+ * Returns with a random open port can be used to set as server port for ZooKeeper.
+ *
+ * @return a random open port or 0 (in case of error)
+ */
+ private def getRandomAvailablePort: Int = {
Review comment:
Got, we can remove `normalizePort` and `getRandomAvailablePort`.
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685870378
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -56,12 +56,14 @@ object ZookeeperConf {
.version("1.2.0")
.fallbackConf(EMBEDDED_ZK_TEMP_DIR)
+ @deprecated(s"the config will be removed", since = "1.3.0")
Review comment:
cc @pan3793
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895954978
Looked into Travis CI, there seems have restrictions.
https://app.travis-ci.com/plans?anchor=enterprise-section
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895740904
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 [#907](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (610db54) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/89d031264b0332aced5a8c104cd14789810cbdea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89d0312) will **decrease** coverage by `0.09%`.
> The diff coverage is `79.41%`.
> :exclamation: Current head 610db54 differs from pull request most recent head a336544. Consider uploading reports for the commit a336544 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907 +/- ##
============================================
- Coverage 79.18% 79.09% -0.10%
Complexity 11 11
============================================
Files 143 143
Lines 5391 5390 -1
Branches 652 657 +5
============================================
- Hits 4269 4263 -6
- Misses 757 759 +2
- Partials 365 368 +3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ommon/src/main/scala/org/apache/kyuubi/Utils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9VdGlscy5zY2FsYQ==) | `69.87% <0.00%> (-3.54%)` | :arrow_down: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `58.49% <ø> (-1.51%)` | :arrow_down: |
| [...rg/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvRW1iZWRkZWRab29rZWVwZXIuc2NhbGE=) | `87.09% <85.71%> (-3.15%)` | :arrow_down: |
| [...la/org/apache/kyuubi/zookeeper/ZookeeperConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvWm9va2VlcGVyQ29uZi5zY2FsYQ==) | `100.00% <100.00%> (ø)` | |
| [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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=) | `79.41% <0.00%> (-0.99%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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/907?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 [89d0312...a336544](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 pull request #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895935516
> By the way, maybe it's time to remove travis ci, it always cost much more time for each commits.
> 
can you check 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] timothy65535 removed a comment on pull request #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 removed a comment on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895785326
BTW, maybe it's time to remove the travis ci, it always cost more time than others for each commit.
--
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] ulysses-you closed pull request #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
ulysses-you closed pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895980843
> maybe we can disable travis on PRs but keep it for commits only
https://docs.travis-ci.com/user/customizing-the-build/#building-specific-branches？
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685969609
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -56,12 +56,14 @@ object ZookeeperConf {
.version("1.2.0")
.fallbackConf(EMBEDDED_ZK_TEMP_DIR)
+ @deprecated(s"the config will be removed", since = "1.3.0")
Review comment:
OK, let's remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895955792
The travis job started after other two canceled.
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895966926
maybe we can disable travis on PRs but keep it for commits only
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895785326
BTW, maybe it's time to remove the travis ci, it always cost more time than others for each commit.
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895740904
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 [#907](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7d7fde9) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/89d031264b0332aced5a8c104cd14789810cbdea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89d0312) will **decrease** coverage by `0.11%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907 +/- ##
============================================
- Coverage 79.18% 79.07% -0.12%
Complexity 11 11
============================================
Files 143 145 +2
Lines 5391 5405 +14
Branches 652 656 +4
============================================
+ Hits 4269 4274 +5
- Misses 757 762 +5
- Partials 365 369 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ommon/src/main/scala/org/apache/kyuubi/Utils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9VdGlscy5zY2FsYQ==) | `70.23% <0.00%> (-3.18%)` | :arrow_down: |
| [...rg/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvRW1iZWRkZWRab29rZWVwZXIuc2NhbGE=) | `86.66% <85.00%> (-3.58%)` | :arrow_down: |
| [...la/org/apache/kyuubi/zookeeper/ZookeeperConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvWm9va2VlcGVyQ29uZi5zY2FsYQ==) | `100.00% <100.00%> (ø)` | |
| [...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9TcGFya1NRTEVuZ2luZS5zY2FsYQ==) | `69.69% <0.00%> (-4.28%)` | :arrow_down: |
| [.../engine/spark/session/SparkSQLSessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9zZXNzaW9uL1NwYXJrU1FMU2Vzc2lvbk1hbmFnZXIuc2NhbGE=) | `91.66% <0.00%> (-3.08%)` | :arrow_down: |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `58.49% <0.00%> (-1.51%)` | :arrow_down: |
| [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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=) | `79.41% <0.00%> (-0.99%)` | :arrow_down: |
| [...pache/kyuubi/engine/spark/events/EngineEvent.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvRW5naW5lRXZlbnQuc2NhbGE=) | `89.65% <0.00%> (-0.35%)` | :arrow_down: |
| [...kyuubi/engine/spark/session/SparkSessionImpl.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9zZXNzaW9uL1NwYXJrU2Vzc2lvbkltcGwuc2NhbGE=) | `100.00% <0.00%> (ø)` | |
| ... and [8 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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/907?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 [89d0312...7d7fde9](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685857956
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -87,26 +89,34 @@ object ZookeeperConf {
.intConf
.createWithDefault(120)
- val ZK_MIN_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MIN_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.min.session.timeout")
.doc("minSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 2 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 2)
- val ZK_MAX_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MAX_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.max.session.timeout")
.doc("maxSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 20 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 20)
+ @deprecated(s"the config will be removed", since = "1.3.0")
val ZK_SERVER_ID: ConfigEntry[Int] =
buildConf("zookeeper.embedded.server.id")
.doc("serverId for the embedded zookeeper server")
.version("1.2.0")
.intConf
.createWithDefault(-1)
+
+ val ZK_DELETE_DATA_DIRECTORY_ON_CLOSE: ConfigEntry[Boolean] =
+ buildConf("zookeeper.embedded.delete-data-directory-on-close")
Review comment:
hi, you mean always let `ZK_DELETE_DATA_DIRECTORY_ON_CLOSE` = true ?
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685972520
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -56,12 +56,14 @@ object ZookeeperConf {
.version("1.2.0")
.fallbackConf(EMBEDDED_ZK_TEMP_DIR)
+ @deprecated(s"the config will be removed", since = "1.3.0")
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895740904
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 [#907](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (400d8e1) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/89d031264b0332aced5a8c104cd14789810cbdea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89d0312) will **decrease** coverage by `0.06%`.
> The diff coverage is `89.36%`.
> :exclamation: Current head 400d8e1 differs from pull request most recent head ce06a0f. Consider uploading reports for the commit ce06a0f to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907 +/- ##
============================================
- Coverage 79.18% 79.12% -0.07%
Complexity 11 11
============================================
Files 143 145 +2
Lines 5391 5418 +27
Branches 652 656 +4
============================================
+ Hits 4269 4287 +18
- Misses 757 762 +5
- Partials 365 369 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pache/kyuubi/engine/spark/events/EngineEvent.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvRW5naW5lRXZlbnQuc2NhbGE=) | `89.65% <ø> (-0.35%)` | :arrow_down: |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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% <ø> (ø)` | |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `58.49% <ø> (-1.51%)` | :arrow_down: |
| [...ommon/src/main/scala/org/apache/kyuubi/Utils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9VdGlscy5zY2FsYQ==) | `70.23% <33.33%> (-3.18%)` | :arrow_down: |
| [...rg/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvRW1iZWRkZWRab29rZWVwZXIuc2NhbGE=) | `86.66% <85.00%> (-3.58%)` | :arrow_down: |
| [.../engine/spark/session/SparkSQLSessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9zZXNzaW9uL1NwYXJrU1FMU2Vzc2lvbk1hbmFnZXIuc2NhbGE=) | `91.66% <85.71%> (-3.08%)` | :arrow_down: |
| [...ache/kyuubi/engine/spark/events/SessionEvent.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvU2Vzc2lvbkV2ZW50LnNjYWxh) | `88.23% <88.23%> (ø)` | |
| [...g/apache/kyuubi/engine/spark/KyuubiSparkUtil.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9LeXV1YmlTcGFya1V0aWwuc2NhbGE=) | `91.30% <100.00%> (+0.39%)` | :arrow_up: |
| [...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9TcGFya1NRTEVuZ2luZS5zY2FsYQ==) | `69.69% <100.00%> (-4.28%)` | :arrow_down: |
| [...uubi/engine/spark/events/EventLoggingService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvRXZlbnRMb2dnaW5nU2VydmljZS5zY2FsYQ==) | `87.50% <100.00%> (+4.89%)` | :arrow_up: |
| ... and [13 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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/907?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 [89d0312...ce06a0f](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895740904
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 [#907](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (02ad022) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/89d031264b0332aced5a8c104cd14789810cbdea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89d0312) will **decrease** coverage by `0.10%`.
> The diff coverage is `79.41%`.
> :exclamation: Current head 02ad022 differs from pull request most recent head a336544. Consider uploading reports for the commit a336544 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907 +/- ##
============================================
- Coverage 79.18% 79.08% -0.11%
Complexity 11 11
============================================
Files 143 143
Lines 5391 5392 +1
Branches 652 657 +5
============================================
- Hits 4269 4264 -5
- Misses 757 760 +3
- Partials 365 368 +3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ommon/src/main/scala/org/apache/kyuubi/Utils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9VdGlscy5zY2FsYQ==) | `69.87% <0.00%> (-3.54%)` | :arrow_down: |
| [...rg/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvRW1iZWRkZWRab29rZWVwZXIuc2NhbGE=) | `87.09% <85.71%> (-3.15%)` | :arrow_down: |
| [...la/org/apache/kyuubi/zookeeper/ZookeeperConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvWm9va2VlcGVyQ29uZi5zY2FsYQ==) | `100.00% <100.00%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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: |
| [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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=) | `79.41% <0.00%> (-0.99%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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/907?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 [89d0312...a336544](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685855897
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -87,26 +89,34 @@ object ZookeeperConf {
.intConf
.createWithDefault(120)
- val ZK_MIN_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MIN_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.min.session.timeout")
.doc("minSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 2 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 2)
- val ZK_MAX_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MAX_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.max.session.timeout")
.doc("maxSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 20 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 20)
+ @deprecated(s"the config will be removed", since = "1.3.0")
val ZK_SERVER_ID: ConfigEntry[Int] =
buildConf("zookeeper.embedded.server.id")
.doc("serverId for the embedded zookeeper server")
.version("1.2.0")
.intConf
.createWithDefault(-1)
+
+ val ZK_DELETE_DATA_DIRECTORY_ON_CLOSE: ConfigEntry[Boolean] =
+ buildConf("zookeeper.embedded.delete-data-directory-on-close")
Review comment:
Can we remove this？and keep it AS-IS？
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895972324
> maybe we can disable travis on PRs but keep it for commits only
hi, from following doc, need maintainers disable `Build on Pull Requests` in repository settings.
```
To only build on push events not on pull requests, disable Build on Pull Requests in your repository settings.
```
https://docs.travis-ci.com/user/pull-requests/#how-pull-requests-are-built
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685034063
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala
##########
@@ -17,77 +17,83 @@
package org.apache.kyuubi.zookeeper
-import java.io.File
-import java.net.InetAddress
+import java.io.{File, IOException}
+import java.net.{InetAddress, InetSocketAddress, ServerSocket}
-import scala.collection.JavaConverters._
-
-import org.apache.curator.test.{InstanceSpec, TestingServer}
+import org.apache.zookeeper.server.{NIOServerCnxnFactory, ZooKeeperServer}
import org.apache.kyuubi.Utils._
import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.service.{AbstractService, ServiceState}
import org.apache.kyuubi.zookeeper.ZookeeperConf._
class EmbeddedZookeeper extends AbstractService("EmbeddedZookeeper") {
- private var spec: InstanceSpec = _
- private var server: TestingServer = _
- // When the client port is 0, the TestingServer will not randomly pick free local port to use
- // So adjust it to -1 to achieve what is common cognition.
- private def normalizePort(port: Int): Int = if (port == 0) -1 else port
+ private var zks: ZooKeeperServer = _
+ private var serverFactory: NIOServerCnxnFactory = _
+ private var dataDirectory: File = _
+ private var deleteDataDirectoryOnClose: Boolean = _
override def initialize(conf: KyuubiConf): Unit = synchronized {
- val dataDirectory = new File(conf.get(ZK_DATA_DIR))
+ dataDirectory = new File(conf.get(ZK_DATA_DIR))
+ deleteDataDirectoryOnClose = conf.get(ZK_DELETE_DATA_DIRECTORY_ON_CLOSE)
val clientPort = normalizePort(conf.get(ZK_CLIENT_PORT))
- val electionPort = normalizePort(conf.get(ZK_ELECTION_PORT))
- val quorumPort = normalizePort(conf.get(ZK_QUORUM_PORT))
- val serverId = conf.get(ZK_SERVER_ID)
val tickTime = conf.get(ZK_TICK_TIME)
val maxClientCnxns = conf.get(ZK_MAX_CLIENT_CONNECTIONS)
- // TODO: Is it right in prod?
- val deleteDataDirectoryOnClose = true
- val customProperties =
- (conf.get(ZK_MIN_SESSION_TIMEOUT).map { timeout =>
- "minSessionTimeout" -> Integer.valueOf(timeout)
- } ++ conf.get(ZK_MAX_SESSION_TIMEOUT).map { timeout =>
- "maxSessionTimeout" -> Integer.valueOf(timeout)
- }).toMap[String, Object].asJava
-
+ val minSessionTimeout = conf.get(ZK_MIN_SESSION_TIMEOUT)
+ val maxSessionTimeout = conf.get(ZK_MAX_SESSION_TIMEOUT)
val hostname = conf.get(ZK_CLIENT_PORT_ADDRESS).map(InetAddress.getByName)
.getOrElse(findLocalInetAddress).getCanonicalHostName
- spec = new InstanceSpec(
- dataDirectory,
- clientPort,
- electionPort,
- quorumPort,
- deleteDataDirectoryOnClose,
- serverId,
- tickTime,
- maxClientCnxns,
- customProperties,
- hostname)
- server = new TestingServer(spec, false)
+
+ zks = new ZooKeeperServer(dataDirectory, dataDirectory, tickTime)
+ zks.setMinSessionTimeout(minSessionTimeout)
+ zks.setMaxSessionTimeout(maxSessionTimeout)
+
+ serverFactory = new NIOServerCnxnFactory
+ serverFactory.configure(new InetSocketAddress(hostname, clientPort), maxClientCnxns)
+
super.initialize(conf)
}
override def start(): Unit = synchronized {
- server.start()
+ serverFactory.startup(zks)
info(s"$getName is started at $getConnectString")
// Stop the EmbeddedZookeeper after the Kyuubi server stopped
- addShutdownHook(() => server.close(), SERVER_SHUTDOWN_PRIORITY - 1)
+ addShutdownHook(() => stop(), SERVER_SHUTDOWN_PRIORITY - 1)
super.start()
}
override def stop(): Unit = synchronized {
if (getServiceState == ServiceState.STARTED) {
- server.close()
+ if (null != serverFactory) serverFactory.shutdown()
+ if (null != zks) zks.shutdown()
+ if (deleteDataDirectoryOnClose) deleteDirectoryRecursively(dataDirectory)
}
super.stop()
}
def getConnectString: String = synchronized {
- assert(spec != null, s"$getName is in $getServiceState")
- spec.getConnectString
+ assert(zks != null, s"$getName is in $getServiceState")
+ s"${serverFactory.getLocalAddress.getHostName}:${serverFactory.getLocalPort}"
+ }
+
+ private def normalizePort(port: Int): Int = if (port <= 0) getRandomAvailablePort else port
+
+ /**
+ * Returns with a random open port can be used to set as server port for ZooKeeper.
+ *
+ * @return a random open port or 0 (in case of error)
+ */
+ private def getRandomAvailablePort: Int = {
Review comment:
why is this related to zk? InetSocketAddress is already supporting specify random port
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r684980998
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -87,26 +75,26 @@ object ZookeeperConf {
.intConf
.createWithDefault(120)
- val ZK_MIN_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MIN_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.min.session.timeout")
.doc("minSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 2 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 2)
- val ZK_MAX_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MAX_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.max.session.timeout")
.doc("maxSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 20 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 20)
- val ZK_SERVER_ID: ConfigEntry[Int] =
- buildConf("zookeeper.embedded.server.id")
- .doc("serverId for the embedded zookeeper server")
+ val ZK_DELETE_DATA_DIRECTORY_ON_CLOSE: ConfigEntry[Boolean] =
+ buildConf("zookeeper.embedded.delete-data-directory-on-close")
+ .doc("delete the storage directory of embedded zookeeper when service close")
.version("1.2.0")
Review comment:
Got.
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -87,26 +75,26 @@ object ZookeeperConf {
.intConf
.createWithDefault(120)
- val ZK_MIN_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MIN_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.min.session.timeout")
.doc("minSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 2 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 2)
- val ZK_MAX_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MAX_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.max.session.timeout")
.doc("maxSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 20 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 20)
- val ZK_SERVER_ID: ConfigEntry[Int] =
Review comment:
Got.
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895984838
> > maybe we can disable travis on PRs but keep it for commits only
>
> [https://docs.travis-ci.com/user/customizing-the-build/#building-specific-branches？](https://docs.travis-ci.com/user/customizing-the-build/#building-specific-branches%EF%BC%9F)
Nice, 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] timothy65535 commented on a change in pull request #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685881080
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -87,26 +89,34 @@ object ZookeeperConf {
.intConf
.createWithDefault(120)
- val ZK_MIN_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MIN_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.min.session.timeout")
.doc("minSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 2 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 2)
- val ZK_MAX_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MAX_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.max.session.timeout")
.doc("maxSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 20 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 20)
+ @deprecated(s"the config will be removed", since = "1.3.0")
val ZK_SERVER_ID: ConfigEntry[Int] =
buildConf("zookeeper.embedded.server.id")
.doc("serverId for the embedded zookeeper server")
.version("1.2.0")
.intConf
.createWithDefault(-1)
+
+ val ZK_DELETE_DATA_DIRECTORY_ON_CLOSE: ConfigEntry[Boolean] =
+ buildConf("zookeeper.embedded.delete-data-directory-on-close")
Review comment:
Got
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895929606
By the way, maybe it's time to remove travis ci, it always cost much more time for each commits.
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685859944
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -87,26 +89,34 @@ object ZookeeperConf {
.intConf
.createWithDefault(120)
- val ZK_MIN_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MIN_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.min.session.timeout")
.doc("minSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 2 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 2)
- val ZK_MAX_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MAX_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.max.session.timeout")
.doc("maxSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 20 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 20)
+ @deprecated(s"the config will be removed", since = "1.3.0")
val ZK_SERVER_ID: ConfigEntry[Int] =
buildConf("zookeeper.embedded.server.id")
.doc("serverId for the embedded zookeeper server")
.version("1.2.0")
.intConf
.createWithDefault(-1)
+
+ val ZK_DELETE_DATA_DIRECTORY_ON_CLOSE: ConfigEntry[Boolean] =
+ buildConf("zookeeper.embedded.delete-data-directory-on-close")
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 pull request #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-896441631
@pan3793 @yaooqinn, thanks for the review and suggestion, patch is updated accordingly and all checks have passed.
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895740904
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 [#907](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8b77c7d) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/89d031264b0332aced5a8c104cd14789810cbdea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89d0312) will **decrease** coverage by `0.09%`.
> The diff coverage is `79.41%`.
> :exclamation: Current head 8b77c7d differs from pull request most recent head a336544. Consider uploading reports for the commit a336544 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907 +/- ##
============================================
- Coverage 79.18% 79.09% -0.10%
Complexity 11 11
============================================
Files 143 143
Lines 5391 5390 -1
Branches 652 657 +5
============================================
- Hits 4269 4263 -6
- Misses 757 759 +2
- Partials 365 368 +3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ommon/src/main/scala/org/apache/kyuubi/Utils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9VdGlscy5zY2FsYQ==) | `69.87% <0.00%> (-3.54%)` | :arrow_down: |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `58.49% <ø> (-1.51%)` | :arrow_down: |
| [...rg/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvRW1iZWRkZWRab29rZWVwZXIuc2NhbGE=) | `87.09% <85.71%> (-3.15%)` | :arrow_down: |
| [...la/org/apache/kyuubi/zookeeper/ZookeeperConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvWm9va2VlcGVyQ29uZi5zY2FsYQ==) | `100.00% <100.00%> (ø)` | |
| [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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=) | `79.41% <0.00%> (-0.99%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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/907?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 [89d0312...a336544](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895740904
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 [#907](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (400d8e1) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/89d031264b0332aced5a8c104cd14789810cbdea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89d0312) will **decrease** coverage by `0.06%`.
> The diff coverage is `89.36%`.
> :exclamation: Current head 400d8e1 differs from pull request most recent head 7d7fde9. Consider uploading reports for the commit 7d7fde9 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907 +/- ##
============================================
- Coverage 79.18% 79.12% -0.07%
Complexity 11 11
============================================
Files 143 145 +2
Lines 5391 5418 +27
Branches 652 656 +4
============================================
+ Hits 4269 4287 +18
- Misses 757 762 +5
- Partials 365 369 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pache/kyuubi/engine/spark/events/EngineEvent.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvRW5naW5lRXZlbnQuc2NhbGE=) | `89.65% <ø> (-0.35%)` | :arrow_down: |
| [...la/org/apache/kyuubi/session/AbstractSession.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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% <ø> (ø)` | |
| [...ache/kyuubi/operation/KyuubiOperationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uTWFuYWdlci5zY2FsYQ==) | `58.49% <ø> (-1.51%)` | :arrow_down: |
| [...ommon/src/main/scala/org/apache/kyuubi/Utils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9VdGlscy5zY2FsYQ==) | `70.23% <33.33%> (-3.18%)` | :arrow_down: |
| [...rg/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvRW1iZWRkZWRab29rZWVwZXIuc2NhbGE=) | `86.66% <85.00%> (-3.58%)` | :arrow_down: |
| [.../engine/spark/session/SparkSQLSessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9zZXNzaW9uL1NwYXJrU1FMU2Vzc2lvbk1hbmFnZXIuc2NhbGE=) | `91.66% <85.71%> (-3.08%)` | :arrow_down: |
| [...ache/kyuubi/engine/spark/events/SessionEvent.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvU2Vzc2lvbkV2ZW50LnNjYWxh) | `88.23% <88.23%> (ø)` | |
| [...g/apache/kyuubi/engine/spark/KyuubiSparkUtil.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9LeXV1YmlTcGFya1V0aWwuc2NhbGE=) | `91.30% <100.00%> (+0.39%)` | :arrow_up: |
| [...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9TcGFya1NRTEVuZ2luZS5zY2FsYQ==) | `69.69% <100.00%> (-4.28%)` | :arrow_down: |
| [...uubi/engine/spark/events/EventLoggingService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvRXZlbnRMb2dnaW5nU2VydmljZS5zY2FsYQ==) | `87.50% <100.00%> (+4.89%)` | :arrow_up: |
| ... and [13 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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/907?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 [89d0312...7d7fde9](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685858732
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -56,12 +56,14 @@ object ZookeeperConf {
.version("1.2.0")
.fallbackConf(EMBEDDED_ZK_TEMP_DIR)
+ @deprecated(s"the config will be removed", since = "1.3.0")
Review comment:
I guess all these configs can be removed since they don't print any warning or blocks anything after being removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685036257
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala
##########
@@ -17,77 +17,83 @@
package org.apache.kyuubi.zookeeper
-import java.io.File
-import java.net.InetAddress
+import java.io.{File, IOException}
+import java.net.{InetAddress, InetSocketAddress, ServerSocket}
-import scala.collection.JavaConverters._
-
-import org.apache.curator.test.{InstanceSpec, TestingServer}
+import org.apache.zookeeper.server.{NIOServerCnxnFactory, ZooKeeperServer}
import org.apache.kyuubi.Utils._
import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.service.{AbstractService, ServiceState}
import org.apache.kyuubi.zookeeper.ZookeeperConf._
class EmbeddedZookeeper extends AbstractService("EmbeddedZookeeper") {
- private var spec: InstanceSpec = _
- private var server: TestingServer = _
- // When the client port is 0, the TestingServer will not randomly pick free local port to use
- // So adjust it to -1 to achieve what is common cognition.
- private def normalizePort(port: Int): Int = if (port == 0) -1 else port
+ private var zks: ZooKeeperServer = _
+ private var serverFactory: NIOServerCnxnFactory = _
+ private var dataDirectory: File = _
+ private var deleteDataDirectoryOnClose: Boolean = _
override def initialize(conf: KyuubiConf): Unit = synchronized {
- val dataDirectory = new File(conf.get(ZK_DATA_DIR))
+ dataDirectory = new File(conf.get(ZK_DATA_DIR))
+ deleteDataDirectoryOnClose = conf.get(ZK_DELETE_DATA_DIRECTORY_ON_CLOSE)
val clientPort = normalizePort(conf.get(ZK_CLIENT_PORT))
- val electionPort = normalizePort(conf.get(ZK_ELECTION_PORT))
- val quorumPort = normalizePort(conf.get(ZK_QUORUM_PORT))
- val serverId = conf.get(ZK_SERVER_ID)
val tickTime = conf.get(ZK_TICK_TIME)
val maxClientCnxns = conf.get(ZK_MAX_CLIENT_CONNECTIONS)
- // TODO: Is it right in prod?
- val deleteDataDirectoryOnClose = true
- val customProperties =
- (conf.get(ZK_MIN_SESSION_TIMEOUT).map { timeout =>
- "minSessionTimeout" -> Integer.valueOf(timeout)
- } ++ conf.get(ZK_MAX_SESSION_TIMEOUT).map { timeout =>
- "maxSessionTimeout" -> Integer.valueOf(timeout)
- }).toMap[String, Object].asJava
-
+ val minSessionTimeout = conf.get(ZK_MIN_SESSION_TIMEOUT)
+ val maxSessionTimeout = conf.get(ZK_MAX_SESSION_TIMEOUT)
val hostname = conf.get(ZK_CLIENT_PORT_ADDRESS).map(InetAddress.getByName)
.getOrElse(findLocalInetAddress).getCanonicalHostName
- spec = new InstanceSpec(
- dataDirectory,
- clientPort,
- electionPort,
- quorumPort,
- deleteDataDirectoryOnClose,
- serverId,
- tickTime,
- maxClientCnxns,
- customProperties,
- hostname)
- server = new TestingServer(spec, false)
+
+ zks = new ZooKeeperServer(dataDirectory, dataDirectory, tickTime)
+ zks.setMinSessionTimeout(minSessionTimeout)
+ zks.setMaxSessionTimeout(maxSessionTimeout)
+
+ serverFactory = new NIOServerCnxnFactory
+ serverFactory.configure(new InetSocketAddress(hostname, clientPort), maxClientCnxns)
+
super.initialize(conf)
}
override def start(): Unit = synchronized {
- server.start()
+ serverFactory.startup(zks)
info(s"$getName is started at $getConnectString")
// Stop the EmbeddedZookeeper after the Kyuubi server stopped
- addShutdownHook(() => server.close(), SERVER_SHUTDOWN_PRIORITY - 1)
+ addShutdownHook(() => stop(), SERVER_SHUTDOWN_PRIORITY - 1)
super.start()
}
override def stop(): Unit = synchronized {
if (getServiceState == ServiceState.STARTED) {
- server.close()
+ if (null != serverFactory) serverFactory.shutdown()
+ if (null != zks) zks.shutdown()
+ if (deleteDataDirectoryOnClose) deleteDirectoryRecursively(dataDirectory)
}
super.stop()
}
def getConnectString: String = synchronized {
- assert(spec != null, s"$getName is in $getServiceState")
- spec.getConnectString
+ assert(zks != null, s"$getName is in $getServiceState")
+ s"${serverFactory.getLocalAddress.getHostName}:${serverFactory.getLocalPort}"
+ }
+
+ private def normalizePort(port: Int): Int = if (port <= 0) getRandomAvailablePort else port
+
+ /**
+ * Returns with a random open port can be used to set as server port for ZooKeeper.
+ *
+ * @return a random open port or 0 (in case of error)
+ */
+ private def getRandomAvailablePort: Int = {
Review comment:
My wrong, is `NIOServerCnxnFactory`. Wait a moment, will look into 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] yaooqinn commented on a change in pull request #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685862016
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/ZookeeperConf.scala
##########
@@ -87,26 +89,34 @@ object ZookeeperConf {
.intConf
.createWithDefault(120)
- val ZK_MIN_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MIN_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.min.session.timeout")
.doc("minSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 2 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 2)
- val ZK_MAX_SESSION_TIMEOUT: OptionalConfigEntry[Int] =
+ val ZK_MAX_SESSION_TIMEOUT: ConfigEntry[Int] =
buildConf("zookeeper.embedded.max.session.timeout")
.doc("maxSessionTimeout in milliseconds for the embedded zookeeper server will allow the" +
" client to negotiate. Defaults to 20 times the tickTime")
.version("1.2.0")
.intConf
- .createOptional
+ .createWithDefault(3000 * 20)
+ @deprecated(s"the config will be removed", since = "1.3.0")
val ZK_SERVER_ID: ConfigEntry[Int] =
buildConf("zookeeper.embedded.server.id")
.doc("serverId for the embedded zookeeper server")
.version("1.2.0")
.intConf
.createWithDefault(-1)
+
+ val ZK_DELETE_DATA_DIRECTORY_ON_CLOSE: ConfigEntry[Boolean] =
+ buildConf("zookeeper.embedded.delete-data-directory-on-close")
Review comment:
Yes, we currently don't have a proper recovery mechanism for the embedded zookeeper
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895741064
> Please also update `LICENSE-binary` and remove `license-binary/LICENSE-javassist.html`, thanks.
Good catch.
--
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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
timothy65535 commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685031187
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala
##########
@@ -17,77 +17,83 @@
package org.apache.kyuubi.zookeeper
-import java.io.File
-import java.net.InetAddress
+import java.io.{File, IOException}
+import java.net.{InetAddress, InetSocketAddress, ServerSocket}
-import scala.collection.JavaConverters._
-
-import org.apache.curator.test.{InstanceSpec, TestingServer}
+import org.apache.zookeeper.server.{NIOServerCnxnFactory, ZooKeeperServer}
import org.apache.kyuubi.Utils._
import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.service.{AbstractService, ServiceState}
import org.apache.kyuubi.zookeeper.ZookeeperConf._
class EmbeddedZookeeper extends AbstractService("EmbeddedZookeeper") {
- private var spec: InstanceSpec = _
- private var server: TestingServer = _
- // When the client port is 0, the TestingServer will not randomly pick free local port to use
- // So adjust it to -1 to achieve what is common cognition.
- private def normalizePort(port: Int): Int = if (port == 0) -1 else port
+ private var zks: ZooKeeperServer = _
+ private var serverFactory: NIOServerCnxnFactory = _
+ private var dataDirectory: File = _
+ private var deleteDataDirectoryOnClose: Boolean = _
override def initialize(conf: KyuubiConf): Unit = synchronized {
- val dataDirectory = new File(conf.get(ZK_DATA_DIR))
+ dataDirectory = new File(conf.get(ZK_DATA_DIR))
+ deleteDataDirectoryOnClose = conf.get(ZK_DELETE_DATA_DIRECTORY_ON_CLOSE)
val clientPort = normalizePort(conf.get(ZK_CLIENT_PORT))
- val electionPort = normalizePort(conf.get(ZK_ELECTION_PORT))
- val quorumPort = normalizePort(conf.get(ZK_QUORUM_PORT))
- val serverId = conf.get(ZK_SERVER_ID)
val tickTime = conf.get(ZK_TICK_TIME)
val maxClientCnxns = conf.get(ZK_MAX_CLIENT_CONNECTIONS)
- // TODO: Is it right in prod?
- val deleteDataDirectoryOnClose = true
- val customProperties =
- (conf.get(ZK_MIN_SESSION_TIMEOUT).map { timeout =>
- "minSessionTimeout" -> Integer.valueOf(timeout)
- } ++ conf.get(ZK_MAX_SESSION_TIMEOUT).map { timeout =>
- "maxSessionTimeout" -> Integer.valueOf(timeout)
- }).toMap[String, Object].asJava
-
+ val minSessionTimeout = conf.get(ZK_MIN_SESSION_TIMEOUT)
+ val maxSessionTimeout = conf.get(ZK_MAX_SESSION_TIMEOUT)
val hostname = conf.get(ZK_CLIENT_PORT_ADDRESS).map(InetAddress.getByName)
.getOrElse(findLocalInetAddress).getCanonicalHostName
- spec = new InstanceSpec(
- dataDirectory,
- clientPort,
- electionPort,
- quorumPort,
- deleteDataDirectoryOnClose,
- serverId,
- tickTime,
- maxClientCnxns,
- customProperties,
- hostname)
- server = new TestingServer(spec, false)
+
+ zks = new ZooKeeperServer(dataDirectory, dataDirectory, tickTime)
+ zks.setMinSessionTimeout(minSessionTimeout)
+ zks.setMaxSessionTimeout(maxSessionTimeout)
+
+ serverFactory = new NIOServerCnxnFactory
+ serverFactory.configure(new InetSocketAddress(hostname, clientPort), maxClientCnxns)
+
super.initialize(conf)
}
override def start(): Unit = synchronized {
- server.start()
+ serverFactory.startup(zks)
info(s"$getName is started at $getConnectString")
// Stop the EmbeddedZookeeper after the Kyuubi server stopped
- addShutdownHook(() => server.close(), SERVER_SHUTDOWN_PRIORITY - 1)
+ addShutdownHook(() => stop(), SERVER_SHUTDOWN_PRIORITY - 1)
super.start()
}
override def stop(): Unit = synchronized {
if (getServiceState == ServiceState.STARTED) {
- server.close()
+ if (null != serverFactory) serverFactory.shutdown()
+ if (null != zks) zks.shutdown()
+ if (deleteDataDirectoryOnClose) deleteDirectoryRecursively(dataDirectory)
}
super.stop()
}
def getConnectString: String = synchronized {
- assert(spec != null, s"$getName is in $getServiceState")
- spec.getConnectString
+ assert(zks != null, s"$getName is in $getServiceState")
+ s"${serverFactory.getLocalAddress.getHostName}:${serverFactory.getLocalPort}"
+ }
+
+ private def normalizePort(port: Int): Int = if (port <= 0) getRandomAvailablePort else port
+
+ /**
+ * Returns with a random open port can be used to set as server port for ZooKeeper.
+ *
+ * @return a random open port or 0 (in case of error)
+ */
+ private def getRandomAvailablePort: Int = {
Review comment:
When the client port is -1, the `TestingServer` will randomly pick free local port to use, but `ZooKeeperServer` does not support this.
Also refer to hadoop `MicroZookeeperService.java`.
--
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 pull request #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
pan3793 commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895701767
Please also update `LICENSE-binary` and remove `license-binary/LICENSE-javassist.html`, 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 commented on pull request #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#issuecomment-895740904
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 [#907](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (02ad022) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/89d031264b0332aced5a8c104cd14789810cbdea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89d0312) will **decrease** coverage by `0.10%`.
> The diff coverage is `79.41%`.
> :exclamation: Current head 02ad022 differs from pull request most recent head 3da0bd5. Consider uploading reports for the commit 3da0bd5 to get more accurate results
[](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907 +/- ##
============================================
- Coverage 79.18% 79.08% -0.11%
Complexity 11 11
============================================
Files 143 143
Lines 5391 5392 +1
Branches 652 657 +5
============================================
- Hits 4269 4264 -5
- Misses 757 760 +3
- Partials 365 368 +3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ommon/src/main/scala/org/apache/kyuubi/Utils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9VdGlscy5zY2FsYQ==) | `69.87% <0.00%> (-3.54%)` | :arrow_down: |
| [...rg/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvRW1iZWRkZWRab29rZWVwZXIuc2NhbGE=) | `87.09% <85.71%> (-3.15%)` | :arrow_down: |
| [...la/org/apache/kyuubi/zookeeper/ZookeeperConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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-a3l1dWJpLXpvb2tlZXBlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS96b29rZWVwZXIvWm9va2VlcGVyQ29uZi5zY2FsYQ==) | `100.00% <100.00%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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: |
| [...pache/kyuubi/sql/KyuubiQueryStagePreparation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/907/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=) | `79.41% <0.00%> (-0.99%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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/907?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 [89d0312...3da0bd5](https://codecov.io/gh/apache/incubator-kyuubi/pull/907?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 #907: [KYUUBI #894] Enhance kyuubi-zookeeper module
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #907:
URL: https://github.com/apache/incubator-kyuubi/pull/907#discussion_r685029202
##########
File path: kyuubi-zookeeper/src/main/scala/org/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala
##########
@@ -17,77 +17,83 @@
package org.apache.kyuubi.zookeeper
-import java.io.File
-import java.net.InetAddress
+import java.io.{File, IOException}
+import java.net.{InetAddress, InetSocketAddress, ServerSocket}
-import scala.collection.JavaConverters._
-
-import org.apache.curator.test.{InstanceSpec, TestingServer}
+import org.apache.zookeeper.server.{NIOServerCnxnFactory, ZooKeeperServer}
import org.apache.kyuubi.Utils._
import org.apache.kyuubi.config.KyuubiConf
import org.apache.kyuubi.service.{AbstractService, ServiceState}
import org.apache.kyuubi.zookeeper.ZookeeperConf._
class EmbeddedZookeeper extends AbstractService("EmbeddedZookeeper") {
- private var spec: InstanceSpec = _
- private var server: TestingServer = _
- // When the client port is 0, the TestingServer will not randomly pick free local port to use
- // So adjust it to -1 to achieve what is common cognition.
- private def normalizePort(port: Int): Int = if (port == 0) -1 else port
+ private var zks: ZooKeeperServer = _
+ private var serverFactory: NIOServerCnxnFactory = _
+ private var dataDirectory: File = _
+ private var deleteDataDirectoryOnClose: Boolean = _
override def initialize(conf: KyuubiConf): Unit = synchronized {
- val dataDirectory = new File(conf.get(ZK_DATA_DIR))
+ dataDirectory = new File(conf.get(ZK_DATA_DIR))
+ deleteDataDirectoryOnClose = conf.get(ZK_DELETE_DATA_DIRECTORY_ON_CLOSE)
val clientPort = normalizePort(conf.get(ZK_CLIENT_PORT))
- val electionPort = normalizePort(conf.get(ZK_ELECTION_PORT))
- val quorumPort = normalizePort(conf.get(ZK_QUORUM_PORT))
- val serverId = conf.get(ZK_SERVER_ID)
val tickTime = conf.get(ZK_TICK_TIME)
val maxClientCnxns = conf.get(ZK_MAX_CLIENT_CONNECTIONS)
- // TODO: Is it right in prod?
- val deleteDataDirectoryOnClose = true
- val customProperties =
- (conf.get(ZK_MIN_SESSION_TIMEOUT).map { timeout =>
- "minSessionTimeout" -> Integer.valueOf(timeout)
- } ++ conf.get(ZK_MAX_SESSION_TIMEOUT).map { timeout =>
- "maxSessionTimeout" -> Integer.valueOf(timeout)
- }).toMap[String, Object].asJava
-
+ val minSessionTimeout = conf.get(ZK_MIN_SESSION_TIMEOUT)
+ val maxSessionTimeout = conf.get(ZK_MAX_SESSION_TIMEOUT)
val hostname = conf.get(ZK_CLIENT_PORT_ADDRESS).map(InetAddress.getByName)
.getOrElse(findLocalInetAddress).getCanonicalHostName
- spec = new InstanceSpec(
- dataDirectory,
- clientPort,
- electionPort,
- quorumPort,
- deleteDataDirectoryOnClose,
- serverId,
- tickTime,
- maxClientCnxns,
- customProperties,
- hostname)
- server = new TestingServer(spec, false)
+
+ zks = new ZooKeeperServer(dataDirectory, dataDirectory, tickTime)
+ zks.setMinSessionTimeout(minSessionTimeout)
+ zks.setMaxSessionTimeout(maxSessionTimeout)
+
+ serverFactory = new NIOServerCnxnFactory
+ serverFactory.configure(new InetSocketAddress(hostname, clientPort), maxClientCnxns)
+
super.initialize(conf)
}
override def start(): Unit = synchronized {
- server.start()
+ serverFactory.startup(zks)
info(s"$getName is started at $getConnectString")
// Stop the EmbeddedZookeeper after the Kyuubi server stopped
- addShutdownHook(() => server.close(), SERVER_SHUTDOWN_PRIORITY - 1)
+ addShutdownHook(() => stop(), SERVER_SHUTDOWN_PRIORITY - 1)
super.start()
}
override def stop(): Unit = synchronized {
if (getServiceState == ServiceState.STARTED) {
- server.close()
+ if (null != serverFactory) serverFactory.shutdown()
+ if (null != zks) zks.shutdown()
+ if (deleteDataDirectoryOnClose) deleteDirectoryRecursively(dataDirectory)
}
super.stop()
}
def getConnectString: String = synchronized {
- assert(spec != null, s"$getName is in $getServiceState")
- spec.getConnectString
+ assert(zks != null, s"$getName is in $getServiceState")
+ s"${serverFactory.getLocalAddress.getHostName}:${serverFactory.getLocalPort}"
+ }
+
+ private def normalizePort(port: Int): Int = if (port <= 0) getRandomAvailablePort else port
+
+ /**
+ * Returns with a random open port can be used to set as server port for ZooKeeper.
+ *
+ * @return a random open port or 0 (in case of error)
+ */
+ private def getRandomAvailablePort: Int = {
Review comment:
why do we need this hack?
--
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