[KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations #6342

wForget · 2024-04-28T10:27:15Z

🔍 Description

Issue References 🔗

This pull request fixes #6344

FlinkProcessBuilder specifies yarn.ship-files, yarn.application.name and yarn.tags configurations of kyuubi platform. Sometimes we also need to customize these configurations, so we should prioritize these user configurations.

Describe Your Solution 🔧

FlinkProcessBuilder prioritizes user configurations.

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

added new unit test

Checklist 📝

This patch was not authored or co-authored using Generative Tooling

Be nice. Be informative.

pan3793 · 2024-04-28T11:20:39Z

the patch involves user-facing changes, to un-confuse users and future exploeres, can you elaborate more on the background and behavior change in PR description?

link3280 · 2024-04-28T11:51:34Z

@wForget I have the same question with @pan3793 . Why do users need to override the yarn tags for flink applications and only flink applications? IIUC, the tags are mainly for internal usage which users rarely care about.

wForget · 2024-04-29T01:27:48Z

@wForget I have the same question with @pan3793 . Why do users need to override the yarn tags for flink applications and only flink applications? IIUC, the tags are mainly for internal usage which users rarely care about.

kyuubi may not directly face users, but be connected to the upper application platform. Just like kyuubi adds session id in yarn tags, we also hope to be able to add the upper application platform id into yarn tags, as well as the application name.

wForget · 2024-04-29T01:32:57Z

the patch involves user-facing changes, to un-confuse users and future exploeres, can you elaborate more on the background and behavior change in PR description?

In my scenario this is a bug fix, just before this change if we specified flink.yarn.ship-files=customShipFile it would override the flinkExtraJars specified in kyuubi, I think we should merge them.

link3280

@wForget Now I understand the motivation. I agree we should merge the user-specified configurations with the Kyuubi generated ones. I left some comments inline, PTAL.

link3280 · 2024-04-29T02:39:05Z

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

@@ -115,7 +115,9 @@ object KyuubiApplicationManager {
  }

  private def setupFlinkYarnTag(tag: String, conf: KyuubiConf): Unit = {
-    val originalTag = conf.getOption(FlinkProcessBuilder.YARN_TAG_KEY).map(_ + ",").getOrElse("")
+    val originalTag = conf.getOption(s"flink.${FlinkProcessBuilder.YARN_TAG_KEY}")


Could we avoid the hardcoded flink. prefix?

BTW is this change compatible with Kyuubi's kill-application-by-tag mechanism?

Could we avoid the hardcoded flink. prefix?

Sure, I will add a constant for it.

BTW is this change compatible with Kyuubi's kill-application-by-tag mechanism?

It is compatible, the previous logic also merges user-specified tags, I just added the key prefixed by flink..

wForget · 2024-04-29T03:29:39Z

the patch involves user-facing changes, to un-confuse users and future exploeres, can you elaborate more on the background and behavior change in PR description?

In my scenario this is a bug fix, just before this change if we specified flink.yarn.ship-files=customShipFile it would override the flinkExtraJars specified in kyuubi, I think we should merge them.

Submitted an issue #6344 and recorded error details.

codecov-commenter · 2024-04-29T05:17:57Z

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 58.43%. Comparing base (5cbbdc3) to head (feca972).
Report is 1 commits behind head on master.

Files	Patch %	Lines
...ache/kyuubi/engine/flink/FlinkProcessBuilder.scala	77.77%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6342      +/-   ##
============================================
- Coverage     58.45%   58.43%   -0.02%     
  Complexity       24       24              
============================================
  Files           653      653              
  Lines         39865    39889      +24     
  Branches       5481     5482       +1     
============================================
+ Hits          23303    23310       +7     
- Misses        14073    14075       +2     
- Partials       2489     2504      +15

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

pan3793 · 2024-04-29T10:23:22Z

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

@@ -115,7 +115,10 @@ object KyuubiApplicationManager {
  }

  private def setupFlinkYarnTag(tag: String, conf: KyuubiConf): Unit = {
-    val originalTag = conf.getOption(FlinkProcessBuilder.YARN_TAG_KEY).map(_ + ",").getOrElse("")
+    val originalTag = conf
+      .getOption(s"${FlinkProcessBuilder.FLINK}.${FlinkProcessBuilder.YARN_TAG_KEY}")


even they are same string, it's for different purpose ...

Suggested change

.getOption(s"${FlinkProcessBuilder.FLINK}.${FlinkProcessBuilder.YARN_TAG_KEY}")

.getOption(s"flink.${FlinkProcessBuilder.YARN_TAG_KEY}")

makes sense, let’s add a FLINK_CONF_PREFIXconstant.

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala

pan3793 · 2024-04-29T10:29:04Z

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala

@@ -212,9 +220,12 @@ class FlinkProcessBuilder(
 }

 object FlinkProcessBuilder {
-  final val FLINK_EXEC_FILE = "flink"
+  final val FLINK = "flink"
  final val APP_KEY = "flink.app.name"


why this has a flink. suffix, but others does not?

why this has a flink. suffix, but others does not?

Because this is not a valid flink conf, it should be:

kyuubi/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala

Line 227 in 17df084

final val YARN_APPLICATION_NAME_KEY = "yarn.application.name"

# 🔍 Description ## Issue References 🔗 This pull request fixes #6344 `FlinkProcessBuilder` specifies `yarn.ship-files`, `yarn.application.name` and `yarn.tags` configurations of kyuubi platform. Sometimes we also need to customize these configurations, so we should prioritize these user configurations. ## Describe Your Solution 🔧 FlinkProcessBuilder prioritizes user configurations. ## 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 added new unit test --- # 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 #6342 from wForget/hotfix2. Closes #6344 feca972 [wforget] address comment 17df084 [wforget] fix test and add flink constant ece91cc [wforget] FlinkProcessBuilder prioritizes user configurations Authored-by: wforget <643348094@qq.com> Signed-off-by: wforget <643348094@qq.com> (cherry picked from commit c8e6457) Signed-off-by: wforget <643348094@qq.com>

wForget · 2024-05-14T01:56:04Z

Thanks, merged to master and branch-1.9.

FlinkProcessBuilder prioritizes user configurations

ece91cc

github-actions bot added the module:server label Apr 28, 2024

wForget changed the title ~~FlinkProcessBuilder prioritizes user configurations~~ [KYUUBI #6342] FlinkProcessBuilder prioritizes user configurations Apr 28, 2024

wForget self-assigned this Apr 28, 2024

wForget requested review from link3280 and pan3793 and removed request for link3280 April 28, 2024 10:28

link3280 reviewed Apr 29, 2024

View reviewed changes

fix test and add flink constant

17df084

wForget changed the title ~~[KYUUBI #6342] FlinkProcessBuilder prioritizes user configurations~~ [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations Apr 29, 2024

wForget requested a review from link3280 April 29, 2024 07:12

pan3793 reviewed Apr 29, 2024

View reviewed changes

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala Outdated Show resolved Hide resolved

pan3793 reviewed Apr 29, 2024

View reviewed changes

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala Outdated Show resolved Hide resolved

pan3793 reviewed Apr 29, 2024

View reviewed changes

address comment

feca972

wForget requested a review from pan3793 May 10, 2024 05:26

pan3793 approved these changes May 11, 2024

View reviewed changes

wForget modified the milestones: v1.10.0, v1.9.1 May 14, 2024

wForget closed this in c8e6457 May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations #6342

[KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations #6342

wForget commented Apr 28, 2024 •

edited

pan3793 commented Apr 28, 2024

link3280 commented Apr 28, 2024

wForget commented Apr 29, 2024

wForget commented Apr 29, 2024 •

edited

link3280 left a comment

link3280 Apr 29, 2024 •

edited

link3280 Apr 29, 2024 •

edited

wForget Apr 29, 2024

wForget commented Apr 29, 2024

codecov-commenter commented Apr 29, 2024 •

edited

pan3793 Apr 29, 2024

wForget Apr 30, 2024

pan3793 Apr 29, 2024

wForget Apr 30, 2024

wForget commented May 14, 2024

	.getOption(s"${FlinkProcessBuilder.FLINK}.${FlinkProcessBuilder.YARN_TAG_KEY}")
	.getOption(s"flink.${FlinkProcessBuilder.YARN_TAG_KEY}")

[KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations #6342

[KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations #6342

Conversation

wForget commented Apr 28, 2024 • 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

Checklist 📝

pan3793 commented Apr 28, 2024

link3280 commented Apr 28, 2024

wForget commented Apr 29, 2024

wForget commented Apr 29, 2024 • edited

link3280 left a comment

Choose a reason for hiding this comment

link3280 Apr 29, 2024 • edited

Choose a reason for hiding this comment

link3280 Apr 29, 2024 • edited

Choose a reason for hiding this comment

wForget Apr 29, 2024

Choose a reason for hiding this comment

wForget commented Apr 29, 2024

codecov-commenter commented Apr 29, 2024 • edited

Codecov Report

pan3793 Apr 29, 2024

Choose a reason for hiding this comment

wForget Apr 30, 2024

Choose a reason for hiding this comment

pan3793 Apr 29, 2024

Choose a reason for hiding this comment

wForget Apr 30, 2024

Choose a reason for hiding this comment

wForget commented May 14, 2024

wForget commented Apr 28, 2024 •

edited

wForget commented Apr 29, 2024 •

edited

link3280 Apr 29, 2024 •

edited

link3280 Apr 29, 2024 •

edited

codecov-commenter commented Apr 29, 2024 •

edited