You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "wForget (via GitHub)" <gi...@apache.org> on 2024/04/28 10:27:18 UTC

[PR] FlinkProcessBuilder prioritizes user configurations [kyuubi]

wForget opened a new pull request, #6342:
URL: https://github.com/apache/kyuubi/pull/6342

   # :mag: Description
   ## Issue References πŸ”—
   <!-- Append the issue number after #. If there is no issue for you to link create one or -->
   <!-- If there are no issues to link, please provide details here. -->
   
   This pull request fixes #
   
   ## Describe Your Solution πŸ”§
   
   FlinkProcessBuilder prioritizes user configurations.
   
   
   ## Types of changes :bookmark:
   <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
   - [ ] Bugfix (non-breaking change which fixes an issue)
   - [X] 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 :coffin:
   
   
   #### Behavior With This Pull Request :tada:
   
   
   #### Related Unit Tests
   
   added new unit test
   
   ---
   
   # Checklist πŸ“
   <!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
   <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
   
   - [X] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)
   
   **Be nice. Be informative.**
   


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

Re: [PR] [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#issuecomment-2081832842

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


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

Re: [PR] [KYUUBI #6342] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#issuecomment-2081752980

   > @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.


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

Re: [PR] [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#discussion_r1582834063


##########
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"

Review Comment:
   let's keep the original name, and avoid abusing



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

Re: [PR] [KYUUBI #6342] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#issuecomment-2081756559

   > 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?
   
   
   
   > 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.


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

Re: [PR] [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget closed pull request #6342: [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations
URL: https://github.com/apache/kyuubi/pull/6342


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

Re: [PR] [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#discussion_r1582831798


##########
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}")

Review Comment:
   even they are same string, it's for different purpose ...
   ```suggestion
         .getOption(s"flink.${FlinkProcessBuilder.YARN_TAG_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

Re: [PR] [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#discussion_r1582837847


##########
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"

Review Comment:
   why this has a `flink.` suffix, but others does not?



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

Re: [PR] [KYUUBI #6342] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "link3280 (via GitHub)" <gi...@apache.org>.
link3280 commented on code in PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#discussion_r1582494365


##########
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}")

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



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

Re: [PR] [KYUUBI #6342] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#discussion_r1582498366


##########
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}")

Review Comment:
   > 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.`.



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

Re: [PR] [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#issuecomment-2081905720

   ## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/6342?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   Attention: Patch coverage is `93.33333%` with `1 lines` in your changes are missing coverage. Please review.
   > Project coverage is 58.40%. Comparing base [(`5cbbdc3`)](https://app.codecov.io/gh/apache/kyuubi/commit/5cbbdc32dbc74b7ab2a1cd6d71a7d0af07945494?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`17df084`)](https://app.codecov.io/gh/apache/kyuubi/pull/6342?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   
   | [Files](https://app.codecov.io/gh/apache/kyuubi/pull/6342?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...ache/kyuubi/engine/flink/FlinkProcessBuilder.scala](https://app.codecov.io/gh/apache/kyuubi/pull/6342?src=pr&el=tree&filepath=kyuubi-server%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fkyuubi%2Fengine%2Fflink%2FFlinkProcessBuilder.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvZmxpbmsvRmxpbmtQcm9jZXNzQnVpbGRlci5zY2FsYQ==) | 90.90% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/kyuubi/pull/6342?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #6342      +/-   ##
   ============================================
   - Coverage     58.45%   58.40%   -0.06%     
     Complexity       24       24              
   ============================================
     Files           653      653              
     Lines         39865    39875      +10     
     Branches       5481     5482       +1     
   ============================================
   - Hits          23303    23287      -16     
   - Misses        14073    14088      +15     
   - Partials       2489     2500      +11     
   ```
   
   
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/kyuubi/pull/6342?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   


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

Re: [PR] [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#discussion_r1582837047


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -115,19 +115,27 @@ class FlinkProcessBuilder(
           flinkExtraJars += s"$hiveConfFile"
         }
 
+        val customFlinkConf = conf.getAllWithPrefix(FLINK, "")
+        // add custom yarn.ship-files
+        flinkExtraJars ++= customFlinkConf.get(YARN_SHIP_FILES_KEY)
+        val yarnAppName = customFlinkConf.get(YARN_APPLICATION_NAME_KEY)
+          .orElse(conf.getOption(APP_KEY))
         buffer += "-t"
         buffer += "yarn-application"
         buffer += s"-Dyarn.ship-files=${flinkExtraJars.mkString(";")}"
-        buffer += s"-Dyarn.application.name=${conf.getOption(APP_KEY).get}"
+        buffer += s"-Dyarn.application.name=${yarnAppName.get}"
         buffer += s"-Dyarn.tags=${conf.getOption(YARN_TAG_KEY).get}"
         buffer += "-Dcontainerized.master.env.FLINK_CONF_DIR=."
 
         hiveConfDirOpt.foreach { _ =>
           buffer += "-Dcontainerized.master.env.HIVE_CONF_DIR=."
         }
 
-        val customFlinkConf = conf.getAllWithPrefix("flink", "")
-        customFlinkConf.filter(_._1 != "app.name").foreach { case (k, v) =>
+        customFlinkConf.filter {
+          c =>
+            !Seq("app.name", YARN_SHIP_FILES_KEY, YARN_APPLICATION_NAME_KEY, YARN_TAG_KEY).contains(
+              c._1)

Review Comment:
   ```suggestion
           customFlinkConf.filter { case (k, _) =>
             !Seq("app.name", YARN_SHIP_FILES_KEY, YARN_APPLICATION_NAME_KEY, YARN_TAG_KEY).contains(k)
   ```



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

Re: [PR] [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#discussion_r1584025383


##########
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"

Review Comment:
   > why this has a `flink.` suffix, but others does not?
   
   Because this is not a valid flink conf, it should be:
   
   https://github.com/apache/kyuubi/blob/17df0844d06c5104f45735cf35dc45c0549253b3/kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala#L227



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

Re: [PR] [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#issuecomment-2109131401

   Thanks, merged to master and branch-1.9.


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

Re: [PR] [KYUUBI #6342] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "link3280 (via GitHub)" <gi...@apache.org>.
link3280 commented on code in PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#discussion_r1582493728


##########
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}")

Review Comment:
   Could we avoid the hardcode `flink.` prefix?



##########
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}")

Review Comment:
   Is this change compatible with Kyuubi's kill-application-by-tag mechanism?



##########
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}")

Review Comment:
   Could we avoid the hardcoded `flink.` prefix?



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

Re: [PR] [KYUUBI #6342] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#issuecomment-2081439685

   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?


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

Re: [PR] [KYUUBI #6342] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "link3280 (via GitHub)" <gi...@apache.org>.
link3280 commented on PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#issuecomment-2081447319

   @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. 


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

Re: [PR] [KYUUBI #6344] FlinkProcessBuilder prioritizes user configurations [kyuubi]

Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #6342:
URL: https://github.com/apache/kyuubi/pull/6342#discussion_r1584026649


##########
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}")

Review Comment:
   makes sense, let’s add a `FLINK_CONF_PREFIX`constant.



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