You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/12/15 06:06:55 UTC
[GitHub] [incubator-kyuubi] bowenliang123 opened a new pull request, #3983: support reload server config from config file 1. add reloadServerConf…
bowenliang123 opened a new pull request, #3983:
URL: https://github.com/apache/incubator-kyuubi/pull/3983
… in KyuubiServer to read config and put all to config, 2. add "refresh/server_conf" api to AdminRestApi, 3. add config type "serverConf" in kyuubi-admin cli
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
to close #3982 .
support reload server config from config file
1. add reloadServerConf in KyuubiServer to read config and put all to config,
3. add "refresh/server_conf" api to AdminRestApi,
3. add config type "serverConf" in kyuubi-admin cli
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce reloading server config without restart
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3983:
URL: https://github.com/apache/incubator-kyuubi/pull/3983#discussion_r1049256540
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala:
##########
@@ -105,6 +105,10 @@ object KyuubiServer extends Logging {
val _hadoopConf = KyuubiHadoopUtils.newHadoopConf(new KyuubiConf().loadFileDefaults())
hadoopConf = _hadoopConf
}
+
+ private[kyuubi] def reloadServerConf(): Unit = synchronized {
+ kyuubiServer.conf.loadFileDefaults()
Review Comment:
@turboFei @pan3793
Have a look at this PR if you have time.
It's reusing the `loadFileDefaults` method to read the config file `kyuubi-defaults.conf` and overriding server configs. And if any configs not in the config file are set during runtime, they won't be affected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#discussion_r1060398129
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala:
##########
@@ -105,6 +105,23 @@ object KyuubiServer extends Logging {
val _hadoopConf = KyuubiHadoopUtils.newHadoopConf(new KyuubiConf().loadFileDefaults())
hadoopConf = _hadoopConf
}
+
+ private[kyuubi] def refreshUserDefaultsConf(): Unit = kyuubiServer.conf.synchronized {
+ kyuubiServer.conf.loadFileDefaults()
+ val newLoadedUserDefaults =
+ new KyuubiConf(true).getAllWithPrefix(USER_DEFAULTS_CONF_QUOTE, "")
+ val existedUserDefaults = kyuubiServer.conf.getAllWithPrefix(USER_DEFAULTS_CONF_QUOTE, "")
+
+ for ((k, _) <- existedUserDefaults) {
+ if (!newLoadedUserDefaults.contains(k)) {
+ kyuubiServer.conf.unset(k)
+ }
+ }
+
+ for ((k, v) <- newLoadedUserDefaults) {
+ kyuubiServer.conf.set(k, v)
Review Comment:
Emm... is this implementation correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce reloading server config without restart
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3983:
URL: https://github.com/apache/incubator-kyuubi/pull/3983#issuecomment-1352850812
Or shall we narrow down to allow refresh the following configs,
- user defaults config with prefix of `___${user}.*`
- engine related config with prefix of `spark.*` , `flink.*` and etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce reloading server config without restart
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3983:
URL: https://github.com/apache/incubator-kyuubi/pull/3983#issuecomment-1352775764
I am open to refreshing server conf manually or automatically. Auto-refreshed cache with intervals provided is also good to me.
Considered replacing `settings` from map to Guava cache. There's one thing that bothered me when getting evicted key from the cache loader, it will cause extra repeated IO load of reading the whole config file for each key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce reloading server config without restart
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#issuecomment-1369321846
> I would like to change this PR for reloading users' default configs (with `"___${user}___"` prefix) ...
SGTM, I suppose this way does not introduce side effects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#discussion_r1060406707
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala:
##########
@@ -105,6 +105,23 @@ object KyuubiServer extends Logging {
val _hadoopConf = KyuubiHadoopUtils.newHadoopConf(new KyuubiConf().loadFileDefaults())
hadoopConf = _hadoopConf
}
+
+ private[kyuubi] def refreshUserDefaultsConf(): Unit = kyuubiServer.conf.synchronized {
+ kyuubiServer.conf.loadFileDefaults()
Review Comment:
Sorry for this mistake. legacy code from the initial version for refreshing all server configs.
Addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce reloading server config without restart
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3983:
URL: https://github.com/apache/incubator-kyuubi/pull/3983#issuecomment-1352765990
An alternative option is wrapper `settings` and `reader` of `KyuubiConf` by Guava cache, WDYT? cc @yaooqinn @ulysses-you @turboFei
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you closed pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
ulysses-you closed pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
URL: https://github.com/apache/kyuubi/pull/3983
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#issuecomment-1369433982
> I would like to change this PR for reloading users' default configs (with `"___${user}___"` prefix) this time rather than all the server configs, to limit the impact for engine instance launching.
>
> For `Reconfigurable` support in each config, it could be introduced later with a detailed design and related mechanism for event or restarting components.
cc @pan3793 @ulysses-you @turboFei
I have changed this PR to refreshing user defaults configs, please have a check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#discussion_r1060393979
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/AdminResource.scala:
##########
@@ -64,6 +64,26 @@ private[v1] class AdminResource extends ApiRequestContext with Logging {
Response.ok(s"Refresh the hadoop conf for ${fe.connectionUrl} successfully.").build()
}
+ @ApiResponse(
+ responseCode = "200",
+ content = Array(new Content(
+ mediaType = MediaType.APPLICATION_JSON)),
+ description = "refresh the user defaults configs")
+ @POST
+ @Path("refresh/user_defaults_conf")
+ def refreshUserDefaultsConf(): Response = {
Review Comment:
shall we lock this method ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#discussion_r1060397795
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala:
##########
@@ -105,6 +105,23 @@ object KyuubiServer extends Logging {
val _hadoopConf = KyuubiHadoopUtils.newHadoopConf(new KyuubiConf().loadFileDefaults())
hadoopConf = _hadoopConf
}
+
+ private[kyuubi] def refreshUserDefaultsConf(): Unit = kyuubiServer.conf.synchronized {
+ kyuubiServer.conf.loadFileDefaults()
+ val newLoadedUserDefaults =
+ new KyuubiConf(true).getAllWithPrefix(USER_DEFAULTS_CONF_QUOTE, "")
Review Comment:
Emm... is this implementation correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#discussion_r1060493451
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala:
##########
@@ -105,6 +105,19 @@ object KyuubiServer extends Logging {
val _hadoopConf = KyuubiHadoopUtils.newHadoopConf(new KyuubiConf().loadFileDefaults())
hadoopConf = _hadoopConf
}
+
+ private[kyuubi] def refreshUserDefaultsConf(): Unit = kyuubiServer.conf.synchronized {
+ val existedUserDefaults = kyuubiServer.conf.getAllUserDefaults
+ val newLoadedUserDefaults = new KyuubiConf(true).getAllUserDefaults
Review Comment:
you should call `new KyuubiConf(true).loadFileDefaults`, otherwise nothing would happen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#issuecomment-1370502371
thanks, merging to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce reloading server config without restart
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3983:
URL: https://github.com/apache/incubator-kyuubi/pull/3983#issuecomment-1352651653
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983?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 [#3983](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bf5448e) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/ff6b792745222c80a35ec1aab499d17f6be1df32?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ff6b792) will **increase** coverage by `0.00%`.
> The diff coverage is `80.00%`.
```diff
@@ Coverage Diff @@
## master #3983 +/- ##
=========================================
Coverage 52.06% 52.07%
Complexity 13 13
=========================================
Files 529 529
Lines 29155 29169 +14
Branches 3891 3893 +2
=========================================
+ Hits 15181 15191 +10
- Misses 12589 12594 +5
+ Partials 1385 1384 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../kyuubi/ctl/cmd/refresh/RefreshConfigCommand.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983/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-a3l1dWJpLWN0bC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jdGwvY21kL3JlZnJlc2gvUmVmcmVzaENvbmZpZ0NvbW1hbmQuc2NhbGE=) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
| [...in/java/org/apache/kyuubi/client/AdminRestApi.java](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983/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-a3l1dWJpLXJlc3QtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reXV1YmkvY2xpZW50L0FkbWluUmVzdEFwaS5qYXZh) | `91.30% <0.00%> (-8.70%)` | :arrow_down: |
| [...a/org/apache/kyuubi/ctl/opt/AdminCommandLine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983/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-a3l1dWJpLWN0bC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jdGwvb3B0L0FkbWluQ29tbWFuZExpbmUuc2NhbGE=) | `100.00% <100.00%> (ø)` | |
| [.../scala/org/apache/kyuubi/server/KyuubiServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983/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) | `54.92% <100.00%> (+1.30%)` | :arrow_up: |
| [...rg/apache/kyuubi/server/api/v1/AdminResource.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL3YxL0FkbWluUmVzb3VyY2Uuc2NhbGE=) | `94.93% <100.00%> (+0.65%)` | :arrow_up: |
| [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `80.74% <0.00%> (-0.63%)` | :arrow_down: |
| [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `97.45% <0.00%> (-0.07%)` | :arrow_down: |
| [...g/apache/kyuubi/operation/BatchJobSubmission.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vQmF0Y2hKb2JTdWJtaXNzaW9uLnNjYWxh) | `75.47% <0.00%> (ø)` | |
| [...apache/kyuubi/engine/JpsApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvSnBzQXBwbGljYXRpb25PcGVyYXRpb24uc2NhbGE=) | `77.41% <0.00%> (ø)` | |
| [...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3983/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9ldGNkL0V0Y2REaXNjb3ZlcnlDbGllbnQuc2NhbGE=) | `69.06% <0.00%> (+0.55%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] turboFei commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce reloading server config without restart
Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3983:
URL: https://github.com/apache/incubator-kyuubi/pull/3983#issuecomment-1356014392
how about referring hadoop implementation
https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Reconfigurable.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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce reloading server config without restart
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#issuecomment-1369316314
I would like to change this PR for reloading users' default configs (with `"___${user}___"` prefix) this time rather than all the server configs, to limit the impact for engine instance launching.
For `Reconfigurable` support in each config, it could be introduced later with a detailed design and related mechanism for event or restarting components.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#issuecomment-1369598282
Also, consider backporting this feature to 1.6 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#discussion_r1060396270
##########
kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/opt/AdminCommandLine.scala:
##########
@@ -100,6 +100,6 @@ object AdminCommandLine extends CommonCommandLine {
arg[String]("<configType>")
.optional()
.action((v, c) => c.copy(adminConfigOpts = c.adminConfigOpts.copy(configType = v)))
- .text("The valid config type can be one of the following: hadoopConf."))
+ .text("The valid config type can be one of the following: hadoopConf, userDefaults."))
Review Comment:
OK, let me fix it.
Thus the cli command will become "kyuubi-admin refresh config userDefaultsConf". `conf` occurs twice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#discussion_r1060393487
##########
kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/opt/AdminCommandLine.scala:
##########
@@ -100,6 +100,6 @@ object AdminCommandLine extends CommonCommandLine {
arg[String]("<configType>")
.optional()
.action((v, c) => c.copy(adminConfigOpts = c.adminConfigOpts.copy(configType = v)))
- .text("The valid config type can be one of the following: hadoopConf."))
+ .text("The valid config type can be one of the following: hadoopConf, userDefaults."))
Review Comment:
userDefaults => userDefaultsConf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#discussion_r1061074692
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/AdminResource.scala:
##########
@@ -64,6 +64,26 @@ private[v1] class AdminResource extends ApiRequestContext with Logging {
Response.ok(s"Refresh the hadoop conf for ${fe.connectionUrl} successfully.").build()
}
+ @ApiResponse(
+ responseCode = "200",
+ content = Array(new Content(
+ mediaType = MediaType.APPLICATION_JSON)),
+ description = "refresh the user defaults configs")
+ @POST
+ @Path("refresh/user_defaults_conf")
+ def refreshUserDefaultsConf(): Response = {
+ val userName = fe.getSessionUser(Map.empty[String, String])
+ val ipAddress = fe.getIpAddress
+ info(s"Receive refresh Kyuubi server conf request from $userName/$ipAddress")
+ if (!userName.equals(administrator)) {
+ throw new NotAllowedException(
+ s"$userName is not allowed to refresh the user defaults conf")
+ }
+ info(s"Reloading the Kyuubi server conf")
+ KyuubiServer.refreshUserDefaultsConf()
+ Response.ok(s"Refresh the server conf successfully.").build()
Review Comment:
nit: `Refresh the user defaults conf successfully`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#issuecomment-1369475894
> I'm not against this feature. But a question: can this be covered by `SessionConfAdvisor` ?
`SessionConfAdvisor` was considered and may not be the best solution for this scenario.
1. user defaults configs are prepared and set in `kyuubi-defaults.conf`. From the perspective of ops, changing it as where it is is more straight, and this refreshing feature only helps applying changes without restarting server instance.
2. general usage of `SessionConfAdvisor` is not limit to applying specific default values to conf but also contains some rules for proper conf settings. e.g. fix max mem usage over 4g to 4g at maximum.
3. only one `SessionConfAdvisor` supported right now and it may conflict to other possible usage of `SessionConfAdvisor`
4. user defaults configs may cover larger area than session confs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] ulysses-you commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce reloading server config without restart
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #3983:
URL: https://github.com/apache/incubator-kyuubi/pull/3983#issuecomment-1352828096
I think the first things is: we should make sure what configs can be hot updated. e.g. what happens if user modify the Kyuubi server port or host ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#discussion_r1060396034
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala:
##########
@@ -105,6 +105,23 @@ object KyuubiServer extends Logging {
val _hadoopConf = KyuubiHadoopUtils.newHadoopConf(new KyuubiConf().loadFileDefaults())
hadoopConf = _hadoopConf
}
+
+ private[kyuubi] def refreshUserDefaultsConf(): Unit = kyuubiServer.conf.synchronized {
+ kyuubiServer.conf.loadFileDefaults()
Review Comment:
what does this do ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#issuecomment-1369469128
I'm not against this feature. But a question: can this be covered by `SessionConfAdvisor` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] bowenliang123 commented on a diff in pull request #3983: [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3983:
URL: https://github.com/apache/kyuubi/pull/3983#discussion_r1060408261
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/AdminResource.scala:
##########
@@ -64,6 +64,26 @@ private[v1] class AdminResource extends ApiRequestContext with Logging {
Response.ok(s"Refresh the hadoop conf for ${fe.connectionUrl} successfully.").build()
}
+ @ApiResponse(
+ responseCode = "200",
+ content = Array(new Content(
+ mediaType = MediaType.APPLICATION_JSON)),
+ description = "refresh the user defaults configs")
+ @POST
+ @Path("refresh/user_defaults_conf")
+ def refreshUserDefaultsConf(): Response = {
Review Comment:
`refreshUserDefaultsConf` of KyuubiServer is synchronized on kyuubiServer.conf to prevent concurrent racing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org