Extract common utils for assembling key value pairs with config option prefix in processbuilder by bowenliang123 · Pull Request #5767 · apache/kyuubi

bowenliang123 · 2023-11-24T05:52:13Z

🔍 Description

Issue References 🔗

As described.

Describe Your Solution 🔧

Focus on key points for configuration option assembling, instead of repeating manually command configs assembling
Avoid using magic string value "--conf" / "-cp" in each processbuilder
Extract common utils for assembling key value pairs with config option prefix in processbuilder
Use mutable.ListBuffer for command assembling
Extract common method for redact config value by key names
NO changes in expected string value for processbuilder command assertions in test suites

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 ⚰️

No behavior changes.

Behavior With This Pull Request 🎉

No behavior changes.

Related Unit Tests

Added CommandUtilsSuite.

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.

codecov-commenter · 2023-11-24T17:25:25Z

Codecov Report

Attention: Patch coverage is 81.92771% with 15 lines in your changes missing coverage. Please review.

Project coverage is 61.38%. Comparing base (eb9e88b) to head (b043888).
Report is 707 commits behind head on master.

Files with missing lines	Patch %	Lines
...apache/kyuubi/engine/chat/ChatProcessBuilder.scala	0.00%	8 Missing ⚠️
...ommon/src/main/scala/org/apache/kyuubi/Utils.scala	71.42%	0 Missing and 4 partials ⚠️
.../apache/kyuubi/util/command/CommandLineUtils.scala	84.21%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5767      +/-   ##
============================================
- Coverage     61.40%   61.38%   -0.02%     
  Complexity       23       23              
============================================
  Files           607      608       +1     
  Lines         35946    35931      -15     
  Branches       4937     4937              
============================================
- Hits          22071    22056      -15     
  Misses        11490    11490              
  Partials       2385     2385

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pan3793

LGTM, only minor suggestions

bowenliang123 · 2023-11-29T09:09:05Z

Would you like to double-check this PR? Some changes since your last review. @pan3793

pan3793 · 2023-12-01T14:59:03Z

+    val allConfigs = Seq(
+      batchKyuubiConf.getAll,
+      sparkAppNameConf(),
+      engineLogPathConf(),
+      appendPodNameConf(batchConf)).flatten
+    buffer ++= confKeyValues(allConfigs)


how about?

Suggested change

val allConfigs = Seq(

batchKyuubiConf.getAll,

sparkAppNameConf(),

engineLogPathConf(),

appendPodNameConf(batchConf)).flatten

buffer ++= confKeyValues(allConfigs)

buffer ++= confKeyValues(

batchKyuubiConf.getAll ++

sparkAppNameConf() ++

engineLogPathConf() ++

appendPodNameConf(batchConf))

Have considered the style that you suggest. Maybe I would prefer separating the configs assembling and the command buffer assembling, easier for readability and the further modifications in configs assembling.

that's fine, but prefer to keep ++ instead of Seq.flatten

Sure. Avoided using Seq.flatten.

pan3793 · 2023-12-01T15:06:37Z

overall LGTM, feel free to merge after addressing minor comments and passing CI.
Have good weekend~

bowenliang123 · 2023-12-01T15:07:42Z

overall LGTM, feel free to merge after addressing minor comments and passing CI. Have good weekend~

Thanks for the detailed review on weekend late nights.

…th config option prefix in processbuilder # 🔍 Description ## Issue References 🔗 As described. ## Describe Your Solution 🔧 - Focus on key points for configuration option assembling, instead of repeating manually command configs assembling - Avoid using magic string value "--conf" / "-cp" in each processbuilder - Extract common utils for assembling key value pairs with config option prefix in processbuilder - Use `mutable.ListBuffer` for command assembling - Extract common method for redact config value by key names - NO changes in expected string value for processbuilder command assertions in test suites ## 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 ⚰️ No behavior changes. #### Behavior With This Pull Request 🎉 No behavior changes. #### Related Unit Tests Added `CommandUtilsSuite`. --- # 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 - [x] 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 - [x] 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 #5767 from bowenliang123/config-option. Closes #5767 b043888 [liangbowen] use ++ for command configs 16a3c27 [liangbowen] .key bc28500 [liangbowen] use raw literal in test suites ab018cf [Bowen Liang] config option Lead-authored-by: liangbowen <liangbowen@gf.com.cn> Co-authored-by: Bowen Liang <liangbowen@gf.com.cn> Signed-off-by: liangbowen <liangbowen@gf.com.cn> (cherry picked from commit 13af6ae) Signed-off-by: liangbowen <liangbowen@gf.com.cn>

bowenliang123 · 2023-12-01T18:15:10Z

Thanks, merged to master and branch-1.8 (1.8.1)

…se notes # 🔍 Description ## Issue References 🔗 Currently, we use a rather primitive way to manually write release notes from scratch, and some of the mechanical and repetitive work can be simplified by the scripts. ## Describe Your Solution 🔧 Adds a script to simplify the process of creating release notes. Note: it just simplifies some processes, the release manager still needs to tune the outputs by hand. ## 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 🧪 ``` RELEASE_TAG=v1.8.1 PREVIOUS_RELEASE_TAG=v1.8.0 build/release/pre_gen_release_notes.py ``` ``` $ head build/release/commits-v1.8.1.txt [KYUUBI #5981] Deploy Spark Hive connector with Scala 2.13 to Maven Central [KYUUBI #6058] Make Jetty server stop timeout configurable [KYUUBI #5952][1.8] Disconnect connections without running operations after engine maxlife time graceful period [KYUUBI #6048] Assign serviceNode and add volatile for variables [KYUUBI #5991] Error on reading Atlas properties composed of multi values [KYUUBI #6045] [REST] Sync the AdminRestApi with the AdminResource Apis [KYUUBI #6047] [CI] Free up disk space [KYUUBI #6036] JDBC driver conditional sets fetchSize on opening session [KYUUBI #6028] Exited spark-submit process should not block batch submit queue [KYUUBI #6018] Speed up GetTables operation for Spark session catalog ``` ``` $ head build/release/contributors-v1.8.1.txt * Shaoyun Chen -- [KYUUBI #5857][KYUUBI #5720][KYUUBI #5785][KYUUBI #5617] * Chao Chen -- [KYUUBI #5750] * Flyangz -- [KYUUBI #5832] * Pengqi Li -- [KYUUBI #5713] * Bowen Liang -- [KYUUBI #5730][KYUUBI #5802][KYUUBI #5767][KYUUBI #5831][KYUUBI #5801][KYUUBI #5754][KYUUBI #5626][KYUUBI #5811][KYUUBI #5853][KYUUBI #5765] * Paul Lin -- [KYUUBI #5799][KYUUBI #5814] * Senmiao Liu -- [KYUUBI #5969][KYUUBI #5244] * Xiao Liu -- [KYUUBI #5962] * Peiyue Liu -- [KYUUBI #5331] * Junjie Ma -- [KYUUBI #5789] ``` --- # Checklist 📝 - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes #6074 from pan3793/release-script. Closes #6074 3d5ec20 [Cheng Pan] credits 1765279 [Cheng Pan] Add a script to simplify the process of creating release notes Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org>

… release notes # 🔍 Description ## Issue References 🔗 Currently, we use a rather primitive way to manually write release notes from scratch, and some of the mechanical and repetitive work can be simplified by the scripts. ## Describe Your Solution 🔧 Adds a script to simplify the process of creating release notes. Note: it just simplifies some processes, the release manager still needs to tune the outputs by hand. ## 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 🧪 ``` RELEASE_TAG=v1.8.1 PREVIOUS_RELEASE_TAG=v1.8.0 build/release/pre_gen_release_notes.py ``` ``` $ head build/release/commits-v1.8.1.txt [KYUUBI apache#5981] Deploy Spark Hive connector with Scala 2.13 to Maven Central [KYUUBI apache#6058] Make Jetty server stop timeout configurable [KYUUBI apache#5952][1.8] Disconnect connections without running operations after engine maxlife time graceful period [KYUUBI apache#6048] Assign serviceNode and add volatile for variables [KYUUBI apache#5991] Error on reading Atlas properties composed of multi values [KYUUBI apache#6045] [REST] Sync the AdminRestApi with the AdminResource Apis [KYUUBI apache#6047] [CI] Free up disk space [KYUUBI apache#6036] JDBC driver conditional sets fetchSize on opening session [KYUUBI apache#6028] Exited spark-submit process should not block batch submit queue [KYUUBI apache#6018] Speed up GetTables operation for Spark session catalog ``` ``` $ head build/release/contributors-v1.8.1.txt * Shaoyun Chen -- [KYUUBI apache#5857][KYUUBI apache#5720][KYUUBI apache#5785][KYUUBI apache#5617] * Chao Chen -- [KYUUBI apache#5750] * Flyangz -- [KYUUBI apache#5832] * Pengqi Li -- [KYUUBI apache#5713] * Bowen Liang -- [KYUUBI apache#5730][KYUUBI apache#5802][KYUUBI apache#5767][KYUUBI apache#5831][KYUUBI apache#5801][KYUUBI apache#5754][KYUUBI apache#5626][KYUUBI apache#5811][KYUUBI apache#5853][KYUUBI apache#5765] * Paul Lin -- [KYUUBI apache#5799][KYUUBI apache#5814] * Senmiao Liu -- [KYUUBI apache#5969][KYUUBI apache#5244] * Xiao Liu -- [KYUUBI apache#5962] * Peiyue Liu -- [KYUUBI apache#5331] * Junjie Ma -- [KYUUBI apache#5789] ``` --- # Checklist 📝 - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes apache#6074 from pan3793/release-script. Closes apache#6074 3d5ec20 [Cheng Pan] credits 1765279 [Cheng Pan] Add a script to simplify the process of creating release notes Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org>

turboFei · 2024-04-03T16:17:13Z

      engineLogPathConf() ++
-      appendPodNameConf(batchConf)).foreach { case (k, v) =>
-      buffer += CONF
-      buffer += s"${convertConfigKey(k)}=$v"


broken issue, the key is not converted with convertConfigKey, fix it in #6256

# 🔍 Description ## Issue References 🔗 Convert the spark batch conf key if not start with `spark.`, it is impacted by #5767 ``` protected def convertConfigKey(key: String): String = { if (key.startsWith("spark.")) { key } else if (key.startsWith("hadoop.")) { "spark.hadoop." + key } else { "spark." + key } } ``` ## Describe Your Solution 🔧 ## Types of changes 🔖 - [x] 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 --- # Checklist 📝 - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes #6256 from turboFei/convert_key. Closes #5767 d76655e [Wang, Fei] Fix break Authored-by: Wang, Fei <fwang12@ebay.com> Signed-off-by: Wang, Fei <fwang12@ebay.com>

# 🔍 Description ## Issue References 🔗 Convert the spark batch conf key if not start with `spark.`, it is impacted by #5767 ``` protected def convertConfigKey(key: String): String = { if (key.startsWith("spark.")) { key } else if (key.startsWith("hadoop.")) { "spark.hadoop." + key } else { "spark." + key } } ``` ## Describe Your Solution 🔧 ## Types of changes 🔖 - [x] 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 --- # Checklist 📝 - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes #6256 from turboFei/convert_key. Closes #5767 d76655e [Wang, Fei] Fix break Authored-by: Wang, Fei <fwang12@ebay.com> Signed-off-by: Wang, Fei <fwang12@ebay.com> (cherry picked from commit c8a40d9) Signed-off-by: Wang, Fei <fwang12@ebay.com>

Convert the spark batch conf key if not start with `spark.`, it is impacted by #5767 ``` protected def convertConfigKey(key: String): String = { if (key.startsWith("spark.")) { key } else if (key.startsWith("hadoop.")) { "spark.hadoop." + key } else { "spark." + key } } ``` - [x] 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) --- - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) **Be nice. Be informative.** Closes #6256 from turboFei/convert_key. Closes #5767 d76655e [Wang, Fei] Fix break Authored-by: Wang, Fei <fwang12@ebay.com> Signed-off-by: Wang, Fei <fwang12@ebay.com> (cherry picked from commit c8a40d9) Signed-off-by: Wang, Fei <fwang12@ebay.com>

github-actions Bot added module:server module:flink module:hive module:common labels Nov 24, 2023

bowenliang123 marked this pull request as draft November 24, 2023 05:55

bowenliang123 marked this pull request as ready for review November 24, 2023 06:13

bowenliang123 marked this pull request as draft November 24, 2023 08:54

bowenliang123 force-pushed the config-option branch from b5e706d to 5bb8f48 Compare November 27, 2023 02:05

bowenliang123 marked this pull request as ready for review November 27, 2023 09:45

pan3793 reviewed Nov 27, 2023

View reviewed changes

Comment thread kyuubi-util-scala/src/test/scala/org/apache/kyuubi/util/command/CommandUtilsSuite.scala Outdated

pan3793 reviewed Nov 27, 2023

View reviewed changes

Comment thread kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/command/CommandLineUtils.scala

pan3793 reviewed Nov 27, 2023

View reviewed changes

bowenliang123 force-pushed the config-option branch 2 times, most recently from b623755 to ca86fdd Compare November 28, 2023 03:26

pan3793 reviewed Nov 28, 2023

View reviewed changes

Comment thread kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/command/CommandLineUtils.scala Outdated

pan3793 reviewed Nov 28, 2023

View reviewed changes

Comment thread kyuubi-util-scala/src/main/scala/org/apache/kyuubi/util/command/CommandLineUtils.scala Outdated

pan3793 approved these changes Nov 28, 2023

View reviewed changes

bowenliang123 force-pushed the config-option branch from 347bba1 to cbb4b41 Compare November 28, 2023 16:03

config option

ab018cf

bowenliang123 force-pushed the config-option branch from a445016 to ab018cf Compare November 29, 2023 05:45

pan3793 reviewed Dec 1, 2023

View reviewed changes

Comment thread kyuubi-server/src/test/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilderSuite.scala Outdated

pan3793 reviewed Dec 1, 2023

View reviewed changes

Comment thread kyuubi-server/src/main/scala/org/apache/kyuubi/engine/trino/TrinoProcessBuilder.scala Outdated

use raw literal in test suites

bc28500

pan3793 reviewed Dec 1, 2023

View reviewed changes

bowenliang123 added 2 commits December 1, 2023 23:04

.key

16a3c27

use ++ for command configs

b043888

bowenliang123 self-assigned this Dec 1, 2023

bowenliang123 closed this in 13af6ae Dec 1, 2023

bowenliang123 added this to the v1.8.1 milestone Dec 1, 2023

bowenliang123 deleted the config-option branch December 1, 2023 18:14

turboFei mentioned this pull request Apr 3, 2024

[KYUUBI #5767][FOLLOWUP] Fix spark batch conf not convert issue #6256

Closed

4 tasks

turboFei reviewed Apr 3, 2024

View reviewed changes

Uh oh!

Conversation

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

Uh oh!

🔍 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

Uh oh!

codecov-commenter commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

pan3793 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pan3793 Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan3793 Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

bowenliang123 Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

pan3793 commented Dec 1, 2023

Uh oh!

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

Uh oh!

Uh oh!

bowenliang123 commented Dec 1, 2023

Uh oh!

turboFei Apr 3, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bowenliang123 commented Nov 24, 2023 •

edited

Loading

codecov-commenter commented Nov 24, 2023 •

edited

Loading

bowenliang123 commented Nov 29, 2023 •

edited

Loading

bowenliang123 Dec 1, 2023 •

edited

Loading

bowenliang123 commented Dec 1, 2023 •

edited

Loading