You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "Ocean22 (via GitHub)" <gi...@apache.org> on 2023/11/15 03:00:23 UTC
[PR] [KYUUBI #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Ocean22 opened a new pull request, #5695:
URL: https://github.com/apache/kyuubi/pull/5695
[KYUUBI #5680] Optimize kyuubi spark engine driver and executor pod name generation
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
**Background**:
1) In default case, when kyuubi submit spark sql engine on kubernetes, will generate spark driver pod name with "kyuubi-${app_name}-${engine_ref_id}-driver".
2) And app_name will be "kyuubi_${shareLevel}_${engineType}_${appUser}_${engineRefId}" if not set by user.
3) In result, we may get spark driver pod name with "kyuubi-kyuubi-${shareLevel}-${engineType}-${appUser}-${engineRefId}-${engineRefId}-driver".
4) We were hoping for a more concise and readable name, such as "kyuubi-${shareLevel}-${engineType}-${appUser}-${app_name}-${engineRefId}-driver".
### _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
**1) Before modification：**
Left is unset `spark.app.name`, and right is set to “ocean”
**2) Modify the code**
**3) Build module and get the jar**
**4) Build a new images**
**5) After modification:**
- [ ] [Run test](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request
### _Was this patch authored or co-authored using generative AI tooling?_
<!--
If a generative AI tooling has been used in the process of authoring this patch, please include
phrase 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
No
--
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 #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#discussion_r1398578910
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/util/KubernetesUtils.scala:
##########
@@ -137,6 +137,8 @@ object KubernetesUtils extends Logging {
lazy val resolvedResourceName = s"kyuubi-${getResourceNamePrefix(appName, engineRefId)}-driver"
if (forciblyRewrite || resolvedResourceName.length > DRIVER_POD_NAME_MAX_LENGTH) {
s"kyuubi-$engineRefId-driver"
+ } else if (appName.contains(engineRefId)){
+ getResourceNamePrefix(appName, "driver")
} else {
resolvedResourceName
}
Review Comment:
```suggestion
val resourceNamePrefix = if (appName.contains(engineRefId)) {
getResourceNamePrefix(appName, "")
} else {
getResourceNamePrefix(appName, engineRefId)
}
val resolvedResourceName = if (resourceNamePrefix.startsWith("kyuubi")) {
s"$resourceNamePrefix-driver"
} else {
s"kyuubi-$resourceNamePrefix-driver"
}
if (forciblyRewrite || resolvedResourceName.length > DRIVER_POD_NAME_MAX_LENGTH) {
s"kyuubi-$engineRefId-driver"
} else {
resolvedResourceName
}
```
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #5695: [KYUUBI #5680] Optimize Spark engine pod name generation
URL: https://github.com/apache/kyuubi/pull/5695
--
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 #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Posted by "Ocean22 (via GitHub)" <gi...@apache.org>.
Ocean22 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1817474621
> > @yaooqinn @zwangsheng Hi, following our previous discussion, I have changed a way to handle this issue.
>
> Sorry, it still need some else change,I still need to do more testing
Now is OK, and this photo is the result that I test.
--
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 #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Posted by "Ocean22 (via GitHub)" <gi...@apache.org>.
Ocean22 commented on code in PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#discussion_r1398690325
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/util/KubernetesUtils.scala:
##########
@@ -137,6 +137,8 @@ object KubernetesUtils extends Logging {
lazy val resolvedResourceName = s"kyuubi-${getResourceNamePrefix(appName, engineRefId)}-driver"
if (forciblyRewrite || resolvedResourceName.length > DRIVER_POD_NAME_MAX_LENGTH) {
s"kyuubi-$engineRefId-driver"
+ } else if (appName.contains(engineRefId)){
+ getResourceNamePrefix(appName, "driver")
} else {
resolvedResourceName
}
Review Comment:
>
Yes,you are right.And I tested with your code and it passed, but about `resourceNamePrefix. startsWith ("kyuubi")`, if `spark. app. name` is set to the name starting with kyuubi, it will have the effect of the arrow in the photo. So should it be changed to `resourceNamePrefix. startsWith ("kyuubi-")`?
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/util/KubernetesUtils.scala:
##########
@@ -137,6 +137,8 @@ object KubernetesUtils extends Logging {
lazy val resolvedResourceName = s"kyuubi-${getResourceNamePrefix(appName, engineRefId)}-driver"
if (forciblyRewrite || resolvedResourceName.length > DRIVER_POD_NAME_MAX_LENGTH) {
s"kyuubi-$engineRefId-driver"
+ } else if (appName.contains(engineRefId)){
+ getResourceNamePrefix(appName, "driver")
} else {
resolvedResourceName
}
Review Comment:
>
Yes,you are right.And I tested with your code and it passed, but about `resourceNamePrefix. startsWith ("kyuubi")`, if `spark. app. name` is set to the name starting with kyuubi, it will have the effect of the arrow in the photo. So should it be changed to `resourceNamePrefix. startsWith ("kyuubi-")`?
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1823849493
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/5695?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `6 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`2568b0d`)](https://app.codecov.io/gh/apache/kyuubi/commit/2568b0d29a8c4bf4b555b80290485bda095b3ffd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.46% compared to head [(`b685b60`)](https://app.codecov.io/gh/apache/kyuubi/pull/5695?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.37%.
> Report is 33 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/kyuubi/pull/5695?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...scala/org/apache/kyuubi/util/KubernetesUtils.scala](https://app.codecov.io/gh/apache/kyuubi/pull/5695?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS91dGlsL0t1YmVybmV0ZXNVdGlscy5zY2FsYQ==) | 53.84% | [3 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/kyuubi/pull/5695?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 #5695 +/- ##
============================================
- Coverage 61.46% 61.37% -0.10%
Complexity 23 23
============================================
Files 604 607 +3
Lines 35697 35846 +149
Branches 4889 4920 +31
============================================
+ Hits 21942 22001 +59
- Misses 11380 11461 +81
- Partials 2375 2384 +9
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/kyuubi/pull/5695?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 #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Posted by "Ocean22 (via GitHub)" <gi...@apache.org>.
Ocean22 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1811912205
Thanks for guidance,I will continue to improve it when I have free time.
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "Ocean22 (via GitHub)" <gi...@apache.org>.
Ocean22 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1823869314
> how about this?
>
> ```diff
> - private def getResourceNamePrefix(appName: String, engineRefId: String): String = {
> - s"$appName-$engineRefId"
> + private def getResourceNamePrefix(appName: String, engineRefId: Option[String]): String = {
> + engineRefId.map(refId => s"$appName-$refId").getOrElse(appName)
> ```
Is it right what I modified?
--
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 #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#discussion_r1393690183
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -187,7 +202,7 @@ private[kyuubi] class EngineRef(
conf.set(KYUUBI_ENGINE_SUBMIT_TIME_KEY, String.valueOf(started))
builder = engineType match {
case SPARK_SQL =>
- conf.setIfMissing(SparkProcessBuilder.APP_KEY, defaultEngineName)
+ conf.set(SparkProcessBuilder.APP_KEY, generateAppKey(conf.getOption(SparkProcessBuilder.APP_KEY).getOrElse(null)))
Review Comment:
IMO, if user set spark app name, we should not over write this, so we should keep setIfMissing here.
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1823849588
how about this?
```patch
- private def getResourceNamePrefix(appName: String, engineRefId: String): String = {
- s"$appName-$engineRefId"
+ private def getResourceNamePrefix(appName: String, engineRefId: Option[String]): String = {
+ engineRefId.map(refId => "$appName-$refId").getOrElse(appName)
```
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1837808873
@Ocean22 could you please provide a Public Name and email (which should be linked to your GitHub account), otherwise your contribution will not be counted by GitHub
--
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 #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#discussion_r1398578910
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/util/KubernetesUtils.scala:
##########
@@ -137,6 +137,8 @@ object KubernetesUtils extends Logging {
lazy val resolvedResourceName = s"kyuubi-${getResourceNamePrefix(appName, engineRefId)}-driver"
if (forciblyRewrite || resolvedResourceName.length > DRIVER_POD_NAME_MAX_LENGTH) {
s"kyuubi-$engineRefId-driver"
+ } else if (appName.contains(engineRefId)){
+ getResourceNamePrefix(appName, "driver")
} else {
resolvedResourceName
}
Review Comment:
```suggestion
val resourceNamePrefix = if (appName.contains(engineRefId)) {
getResourceNamePrefix(appName, "")
} else {
getResourceNamePrefix(appName, engineRefId)
}
val resolvedResourceName = if (resourceNamePrefix.startsWith("kyuubi")) {
s"$resourceNamePrefix-driver"
} else {
s"kyuubi-$resourceNamePrefix-driver"
}
if (resolvedResourceName.length <= DRIVER_POD_NAME_MAX_LENGTH) {
resolvedResourceName
} else {
s"kyuubi-$engineRefId-driver"
}
```
--
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 #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Posted by "Ocean22 (via GitHub)" <gi...@apache.org>.
Ocean22 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1817448750
@yaooqinn @zwangsheng Hi, following our previous discussion, I have changed a way to handle this issue.
--
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 #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Posted by "Ocean22 (via GitHub)" <gi...@apache.org>.
Ocean22 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1817456102
> @yaooqinn @zwangsheng Hi, following our previous discussion, I have changed a way to handle this issue.
Sorry, it still need some else change,I still need to do more testing
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "Ocean22 (via GitHub)" <gi...@apache.org>.
Ocean22 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1837734060
> > how about this?
> > ```diff
> > - private def getResourceNamePrefix(appName: String, engineRefId: String): String = {
> > - s"$appName-$engineRefId"
> > + private def getResourceNamePrefix(appName: String, engineRefId: Option[String]): String = {
> > + engineRefId.map(refId => s"$appName-$refId").getOrElse(appName)
> > ```
>
> Is it right what I modified? 
@pan3793 Hi,the code passed the test,and it's the result
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "Ocean22 (via GitHub)" <gi...@apache.org>.
Ocean22 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1837735199
> @Ocean22 sorry I missed the message, it's correct, please push it
Good,I will push it later.
--
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 #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#discussion_r1393690183
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/EngineRef.scala:
##########
@@ -187,7 +202,7 @@ private[kyuubi] class EngineRef(
conf.set(KYUUBI_ENGINE_SUBMIT_TIME_KEY, String.valueOf(started))
builder = engineType match {
case SPARK_SQL =>
- conf.setIfMissing(SparkProcessBuilder.APP_KEY, defaultEngineName)
+ conf.set(SparkProcessBuilder.APP_KEY, generateAppKey(conf.getOption(SparkProcessBuilder.APP_KEY).getOrElse(null)))
Review Comment:
IMO, if user set spark app name, we should not over write this, so let's keep setIfMissing here.
--
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 #5680] Optimize kyuubi spark engine driver&executor pod name generation [kyuubi]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1811768167
Hi @Ocean22, thanks for the PR.
There is also an original purpose for `spark.app.name` generation I want to share. In early versions of kyuubi, I used(just an attemption) it as a unique key for retrieval. However, we do not rely on it for this purpose.
So, since we are now touching this part, I suggest we make it as simple as possible. for example, the appUser is not necessary here as both YARN UI or Spark History UI display with a USER field, uuid is not necessary as it is also in the app tags,
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "Ocean22 (via GitHub)" <gi...@apache.org>.
Ocean22 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1823848133
> <img alt="image" width="759" src="https://user-images.githubusercontent.com/26535726/285107691-e1fdb297-e5be-430e-9b63-9cfc1346d901.png"> there still is a minor issue on the final result
Sorry,I thought this method would avoid this situation.Let me improve it
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1837733949
@Ocean22 sorry I missed the message, it's correct, please push it
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1837825861
Thanks, merged to master/1.8
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#discussion_r1413321200
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/util/KubernetesUtils.scala:
##########
@@ -146,8 +155,17 @@ object KubernetesUtils extends Logging {
appName: String,
engineRefId: String,
forciblyRewrite: Boolean): String = {
- val resolvedResourceName = s"kyuubi-${getResourceNamePrefix(appName, engineRefId)}"
- if (forciblyRewrite || resolvedResourceName.length > EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH) {
+ val resourceNamePrefix = if (appName.contains(engineRefId)) {
+ getResourceNamePrefix(appName, None)
+ } else {
+ getResourceNamePrefix(appName, Some(engineRefId))
+ }
+ val resolvedResourceName = if (resourceNamePrefix.startsWith("kyuubi-")) {
+ s"$resourceNamePrefix"
+ } else {
+ s"kyuubi-$resourceNamePrefix"
+ }
+ if (forciblyRewrite || resolvedResourceName.length > DRIVER_POD_NAME_MAX_LENGTH) {
Review Comment:
```suggestion
if (forciblyRewrite || resolvedResourceName.length > EXECUTOR_POD_NAME_PREFIX_MAX_LENGTH) {
```
--
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 #5680] Optimize Spark engine pod name generation [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #5695:
URL: https://github.com/apache/kyuubi/pull/5695#issuecomment-1823840798
<img width="759" alt="image" src="https://github.com/apache/kyuubi/assets/26535726/e1fdb297-e5be-430e-9b63-9cfc1346d901">
there still is a minor issue on the final result
--
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