Cleanup unused config `kyuubi.frontend.thrift.http.allow.user.substitution` #5912

bowenliang123 · 2023-12-25T01:53:50Z

🔍 Description

Issue References 🔗

As decribed.

Describe Your Solution 🔧

Drop the unused config of KyuubiConf, which are never used and linked in source code and test code:

FRONTEND_THRIFT_HTTP_ALLOW_USER_SUBSTITUTION: kyuubi.frontend.thrift.http.allow.user.substitution

Types of changes 🔖

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklists

📝 Author Self Checklist

My code follows the style guidelines of this project
I have performed a self-review
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
This patch was not authored or co-authored using Generative Tooling

📝 Committer Pre-Merge Checklist

Be nice. Be informative.

wForget · 2023-12-25T02:15:40Z

CREDENTIALS_HADOOP_FS_ENABLED: kyuubi.credentials.hadoopfs.enabled
CREDENTIALS_HIVE_ENABLED: kyuubi.credentials.hive.enabled

They are used.

FYI:

kyuubi/kyuubi-server/src/main/scala/org/apache/kyuubi/credentials/HadoopCredentialsManager.scala

Line 316 in c290dae

private val providerEnabledConfig = "kyuubi.credentials.%s.enabled"

wForget · 2023-12-25T02:19:00Z

ENGINE_HIVE_MAIN_RESOURCE: kyuubi.session.engine.hive.main.resource

it is also used.

FYI:

kyuubi/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala

Line 67 in c290dae

    
           conf.getOption(s"kyuubi.session.engine.$shortName.main.resource").filter { userSpecified =>

wForget · 2023-12-25T02:49:40Z

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala

@@ -743,14 +729,6 @@ object KyuubiConf {
      .toSequence()
      .createWithDefault(Nil)

-  val FRONTEND_THRIFT_HTTP_ALLOW_USER_SUBSTITUTION: ConfigEntry[Boolean] =


@mahmoudbahaa This configuration was introduced in #2815, could you please help us see if it is used?

bowenliang123 · 2023-12-25T02:53:24Z

Thanks for your double-checks. @wForget
Let me skip the kyuubi.credentials.hadoopfs.enabled and kyuubi.session.engine.hive.main.resource.
This PR is not necessary if there's any possible risk for breaking changes.

codecov-commenter · 2023-12-25T03:13:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (86bec4d) 61.54% compared to head (b938058) 61.54%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5912      +/-   ##
============================================
- Coverage     61.54%   61.54%   -0.01%     
  Complexity       23       23              
============================================
  Files           616      616              
  Lines         36387    36382       -5     
  Branches       4978     4978              
============================================
- Hits          22394    22390       -4     
- Misses        11573    11579       +6     
+ Partials       2420     2413       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bowenliang123 · 2023-12-28T07:54:36Z

Thanks, merged to master.
Please inform us if this removal is inappropriate. @mahmoudbahaa

yaooqinn · 2023-12-28T10:02:43Z

BTW, it seems that 📝 Committer Pre-Merge Checklist is not welcomed at all. @bowenliang123

bowenliang123 · 2023-12-28T10:04:31Z

BTW, it seems that 📝 Committer Pre-Merge Checklist is not welcomed at all. @bowenliang123

Agree. It's quite heavy to fill the radio options either in markdown or on the interactive page.

yaooqinn · 2023-12-28T10:06:50Z

Can you send it PR to remove?

bowenliang123 · 2023-12-28T10:09:11Z

I don't have better ideas in mind right now. Maybe we'd better revert it to the previous version of the pre-commit list, and skip the pre-merge list.

…tp.allow.user.substitution` # 🔍 Description ## Issue References 🔗 As decribed. ## Describe Your Solution 🔧 Drop the unused config of KyuubiConf, which are never used and linked in source code and test code: - FRONTEND_THRIFT_HTTP_ALLOW_USER_SUBSTITUTION: `kyuubi.frontend.thrift.http.allow.user.substitution` ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklists ## 📝 Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [ ] Pull request title is okay. - [ ] No license issues. - [ ] Milestone correctly set? - [ ] Test coverage is ok - [ ] Assignees are selected. - [ ] Minimum number of approvals - [ ] No changes are requested **Be nice. Be informative.** Closes apache#5912 from bowenliang123/unused-configs. Closes apache#5912 b938058 [Bowen Liang] update doc 9fd441d [Bowen Liang] fix 9b0b31e [Bowen Liang] cleanup unused config entries in KyuubiConf Authored-by: Bowen Liang <liangbowen@gf.com.cn> Signed-off-by: Bowen Liang <liangbowen@gf.com.cn>

github-actions bot added kind:documentation Documentation is a feature! module:common labels Dec 25, 2023

wForget reviewed Dec 25, 2023

View reviewed changes

bowenliang123 marked this pull request as draft December 25, 2023 02:56

bowenliang123 changed the title ~~Cleanup unused configs of KyuubiConf~~ Cleanup unused config kyuubi.frontend.thrift.http.allow.user.substitution Dec 25, 2023

bowenliang123 marked this pull request as ready for review December 25, 2023 05:35

pan3793 approved these changes Dec 25, 2023

View reviewed changes

cleanup unused config entries in KyuubiConf

9b0b31e

bowenliang123 force-pushed the unused-configs branch from fe3ac2d to 9b0b31e Compare December 25, 2023 08:34

github-actions bot removed the kind:documentation Documentation is a feature! label Dec 25, 2023

bowenliang123 added 2 commits December 25, 2023 16:34

fix

9fd441d

update doc

b938058

github-actions bot added the kind:documentation Documentation is a feature! label Dec 25, 2023

wForget approved these changes Dec 26, 2023

View reviewed changes

bowenliang123 self-assigned this Dec 28, 2023

bowenliang123 added this to the v1.9.0 milestone Dec 28, 2023

bowenliang123 closed this in b1e726d Dec 28, 2023

bowenliang123 deleted the unused-configs branch December 28, 2023 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup unused config `kyuubi.frontend.thrift.http.allow.user.substitution` #5912

Cleanup unused config `kyuubi.frontend.thrift.http.allow.user.substitution` #5912

bowenliang123 commented Dec 25, 2023 •

edited

wForget commented Dec 25, 2023

wForget commented Dec 25, 2023

wForget Dec 25, 2023

bowenliang123 commented Dec 25, 2023 •

edited

codecov-commenter commented Dec 25, 2023 •

edited

bowenliang123 commented Dec 28, 2023

yaooqinn commented Dec 28, 2023 •

edited

bowenliang123 commented Dec 28, 2023

yaooqinn commented Dec 28, 2023

bowenliang123 commented Dec 28, 2023

Cleanup unused config kyuubi.frontend.thrift.http.allow.user.substitution #5912

Cleanup unused config kyuubi.frontend.thrift.http.allow.user.substitution #5912

Conversation

bowenliang123 commented Dec 25, 2023 • edited

🔍 Description

Issue References 🔗

Describe Your Solution 🔧

Types of changes 🔖

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklists

📝 Author Self Checklist

📝 Committer Pre-Merge Checklist

wForget commented Dec 25, 2023

wForget commented Dec 25, 2023

wForget Dec 25, 2023

Choose a reason for hiding this comment

bowenliang123 commented Dec 25, 2023 • edited

codecov-commenter commented Dec 25, 2023 • edited

Codecov Report

bowenliang123 commented Dec 28, 2023

yaooqinn commented Dec 28, 2023 • edited

bowenliang123 commented Dec 28, 2023

yaooqinn commented Dec 28, 2023

bowenliang123 commented Dec 28, 2023

Cleanup unused config `kyuubi.frontend.thrift.http.allow.user.substitution` #5912

Cleanup unused config `kyuubi.frontend.thrift.http.allow.user.substitution` #5912

bowenliang123 commented Dec 25, 2023 •

edited

bowenliang123 commented Dec 25, 2023 •

edited

codecov-commenter commented Dec 25, 2023 •

edited

yaooqinn commented Dec 28, 2023 •

edited