[KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs #3983

bowenliang123 · 2022-12-15T06:06:53Z

Why are the changes needed?

to close #3982 .

Introduce feature of refresh user defaults config (as ___${user}___.* which starts with three continuous underscores "___") from config file via kyuubi-admin cli and refresh/user_defaults_conf Rest API.

add refreshUserDefaultsConf methond in KyuubiServer to read user defautls configs from property file and apply config changes to server's KyuubiConf
add refresh/user_defaults_conf api to AdminRestApi calling refreshUserDefaultsConf of KyuubiServer
add config type userDefautls in kyuubi-admin cli refresh command

This feature will

help to apply user defaults conf without restarting server or losing connections
load latest config for engine launch, e.g. spark related config spark.*

It won't

affect the components already started and using the clone of server conf
affect configs for launched engine instance

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 locally before make a pull request

… 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

kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala

codecov-commenter · 2022-12-15T07:16:51Z

Codecov Report

Merging #3983 (bf5448e) into master (ff6b792) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@            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	Coverage Δ
.../kyuubi/ctl/cmd/refresh/RefreshConfigCommand.scala	`88.88% <0.00%> (-11.12%)`	⬇️
...in/java/org/apache/kyuubi/client/AdminRestApi.java	`91.30% <0.00%> (-8.70%)`	⬇️
...a/org/apache/kyuubi/ctl/opt/AdminCommandLine.scala	`100.00% <100.00%> (ø)`
.../scala/org/apache/kyuubi/server/KyuubiServer.scala	`54.92% <100.00%> (+1.30%)`	⬆️
...rg/apache/kyuubi/server/api/v1/AdminResource.scala	`94.93% <100.00%> (+0.65%)`	⬆️
...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala	`80.74% <0.00%> (-0.63%)`	⬇️
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala	`97.45% <0.00%> (-0.07%)`	⬇️
...g/apache/kyuubi/operation/BatchJobSubmission.scala	`75.47% <0.00%> (ø)`
...apache/kyuubi/engine/JpsApplicationOperation.scala	`77.41% <0.00%> (ø)`
...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala	`69.06% <0.00%> (+0.55%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pan3793 · 2022-12-15T09:16:17Z

An alternative option is wrapper settings and reader of KyuubiConf by Guava cache, WDYT? cc @yaooqinn @ulysses-you @turboFei

bowenliang123 · 2022-12-15T09:25:20Z

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.

ulysses-you · 2022-12-15T10:06:40Z

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 ?

bowenliang123 · 2022-12-15T10:23:06Z

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.

turboFei · 2022-12-17T03:51:21Z

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

### _Why are the changes needed?_ The changes allow to: 1. set Kyuubi configuration files: - `kyuubi-env.sh` - `kyuubi-defaults.conf` - `log4j2.xml` 2. restart (recreate) Kyuubi server pods if configuration files have changed - this probably should be revisited after #3983 ### _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 Closes #4004 from dnskr/helm_conf_files_support. Closes #4004 8367825 [dnskr] [K8S][HELM] Add configuration files support to helm chart Authored-by: dnskr <dnskrv88@gmail.com> Signed-off-by: Cheng Pan <chengpan@apache.org>

bowenliang123 · 2023-01-03T01:46:14Z

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.

pan3793 · 2023-01-03T02:02:07Z

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.

…g userDefaults`

bowenliang123 · 2023-01-03T06:33:39Z

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.

ulysses-you · 2023-01-03T07:44:30Z

I'm not against this feature. But a question: can this be covered by SessionConfAdvisor ?

bowenliang123 · 2023-01-03T07:57:05Z

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.

user defaults configs are prepared and set in kyuubi-defaults.conf. From the perspective of ops, it's more straightforward to change it in the config file as where it is, and this refreshing feature only helps apply changes without restarting the server.
general usage of SessionConfAdvisor is not limited to applying specific default values to conf but also contains some rules for proper conf settings. e.g. fix max driver mem usage applicator over 4g to 4g at maximum.
only one SessionConfAdvisor is supported right now and it may conflict with another possible usage of SessionConfAdvisor
user defaults configs may cover a larger area than session confs

pan3793 · 2023-01-03T09:18:26Z

kyuubi-ctl/src/main/scala/org/apache/kyuubi/ctl/opt/AdminCommandLine.scala

          .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."))


userDefaults => userDefaultsConf

OK, let me fix it.
Thus the cli command will become "kyuubi-admin refresh config userDefaultsConf". conf occurs twice.

ulysses-you · 2023-01-03T09:19:04Z

kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/AdminResource.scala

+    description = "refresh the user defaults configs")
+  @POST
+  @Path("refresh/user_defaults_conf")
+  def refreshUserDefaultsConf(): Response = {


shall we lock this method ?

refreshUserDefaultsConf of KyuubiServer is synchronized on kyuubiServer.conf to prevent concurrent racing.

kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala

…efreshConfigCommandConfigType

…uubiConf`

bowenliang123 · 2023-01-03T10:19:13Z

Also, consider backporting this feature to 1.6 ?

kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala

ulysses-you · 2023-01-04T01:31:04Z

kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/v1/AdminResource.scala

+    }
+    info(s"Reloading the Kyuubi server conf")
+    KyuubiServer.refreshUserDefaultsConf()
+    Response.ok(s"Refresh the server conf successfully.").build()


nit: Refresh the user defaults conf successfully

ulysses-you

lgtm except one minor comment

ulysses-you · 2023-01-04T05:24:02Z

thanks, merging to master

support reload server config from config file 1. add reloadServerConf…

bf5448e

… 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

github-actions bot added module:ctl module:rest-client module:server labels Dec 15, 2022

bowenliang123 changed the title ~~support reload server config from config file 1. add reloadServerConf…~~ [KYUUBI #3982] [FEATURE] introduce reloading server config without restart Dec 15, 2022

bowenliang123 commented Dec 15, 2022

View reviewed changes

kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala Outdated Show resolved Hide resolved

bowenliang123 marked this pull request as ready for review December 15, 2022 06:46

dnskr mentioned this pull request Dec 18, 2022

[K8S][HELM] Add configuration files support to helm chart #4004

Closed

3 tasks

change to refresh users' config. rename cli command to `refresh confi…

c0a358b

…g userDefaults`

github-actions bot added the module:common label Jan 3, 2023

bowenliang123 added 2 commits January 3, 2023 11:36

typo

e5dd5db

update comments

1b046fe

bowenliang123 changed the title ~~[KYUUBI #3982] [FEATURE] introduce reloading server config without restart~~ [KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs Jan 3, 2023

pan3793 reviewed Jan 3, 2023

View reviewed changes

ulysses-you reviewed Jan 3, 2023

View reviewed changes

kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala Outdated Show resolved Hide resolved

fix redundant loadFileDefaults in refreshUserDefaultsConf

c65398b

pan3793 reviewed Jan 3, 2023

View reviewed changes

kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala Show resolved Hide resolved

bowenliang123 added 4 commits January 3, 2023 17:33

rename config refresh option to userDefaultsConf and extracted to R…

ac407bd

…efreshConfigCommandConfigType

fix user defaults key filtering by adding getAllUserDefaults to `Ky…

e405dc8

…uubiConf`

import

619acd2

fix typo

8dbbbcb

ulysses-you reviewed Jan 3, 2023

View reviewed changes

kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiServer.scala Outdated Show resolved Hide resolved

bowenliang123 added 4 commits January 3, 2023 20:21

fix: loadFileDefaults in refreshUserDefaultsConf

0860e3a

add logging for statistics

786cb2a

minor

b47a154

update

b9b80f5

ulysses-you reviewed Jan 4, 2023

View reviewed changes

fix typo

a8fb0bf

ulysses-you closed this in 75d0b7f Jan 4, 2023

ulysses-you assigned bowenliang123 Jan 4, 2023

ulysses-you added this to the v1.7.0 milestone Jan 4, 2023

bowenliang123 deleted the 3982-reload-server-conf branch January 4, 2023 05:25

[KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs #3983

[KYUUBI #3982] [FEATURE] introduce refreshing user defaults configs #3983

Uh oh!

Conversation

bowenliang123 commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are the changes needed?

How was this patch tested?

Uh oh!

Uh oh!

codecov-commenter commented Dec 15, 2022

Codecov Report

Uh oh!

pan3793 commented Dec 15, 2022

Uh oh!

bowenliang123 commented Dec 15, 2022

Uh oh!

ulysses-you commented Dec 15, 2022

Uh oh!

bowenliang123 commented Dec 15, 2022

Uh oh!

turboFei commented Dec 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bowenliang123 commented Jan 3, 2023

Uh oh!

pan3793 commented Jan 3, 2023

Uh oh!

bowenliang123 commented Jan 3, 2023

Uh oh!

ulysses-you commented Jan 3, 2023

Uh oh!

bowenliang123 commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan3793 Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

bowenliang123 Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

ulysses-you Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

bowenliang123 Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bowenliang123 commented Jan 3, 2023

Uh oh!

Uh oh!

ulysses-you Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

ulysses-you left a comment

Choose a reason for hiding this comment

Uh oh!

ulysses-you commented Jan 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bowenliang123 commented Dec 15, 2022 •

edited

Loading

turboFei commented Dec 17, 2022 •

edited

Loading

bowenliang123 commented Jan 3, 2023 •

edited

Loading