You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "ic4y (via GitHub)" <gi...@apache.org> on 2024/05/11 08:24:50 UTC
[PR] [KYUUBI #6358] Fix the incorrect construction of yarnConf [kyuubi]
ic4y opened a new pull request, #6386:
URL: https://github.com/apache/kyuubi/pull/6386
# :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. -->
https://github.com/apache/kyuubi/issues/6385
## Describe Your Solution ๐ง
This PR is just for better explaining this issue and for testing. The fix is quite crude, and there are still many issues not addressed.
## Types of changes :bookmark:
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [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 :coffin:
#### Behavior With This Pull Request :tada:
#### Related Unit Tests
---
# 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 #6358] Fix the incorrect construction of yarnConf [kyuubi]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6386:
URL: https://github.com/apache/kyuubi/pull/6386#issuecomment-2816921387
Thanks for the PR! This PR is being closed due to inactivity. This isn't a judgement on the merit of the PR in any way. If this is still an issue with the latest version of Kyuubi, please reopen it and ask a committer to remove the Stale tag!
Thank you for using 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 #6358] Fix the incorrect construction of yarnConf [kyuubi]
Posted by "ic4y (via GitHub)" <gi...@apache.org>.
ic4y commented on PR #6386:
URL: https://github.com/apache/kyuubi/pull/6386#issuecomment-2105633237
Looking for a pro to fix 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 #6358] Fix the incorrect construction of yarnConf [kyuubi]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #6386: [KYUUBI #6358] Fix the incorrect construction of yarnConf
URL: https://github.com/apache/kyuubi/pull/6386
--
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 #6358] Fix the incorrect construction of yarnConf [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #6386:
URL: https://github.com/apache/kyuubi/pull/6386#discussion_r1597398933
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/YarnApplicationOperation.scala:
##########
@@ -95,74 +95,139 @@ class YarnApplicationOperation extends ApplicationOperation with Logging {
}
}
+ private def withNewYarnClient[T](
+ proxyUser: Option[String],
+ sessionConf: Option[KyuubiConf])(action: YarnClient => T): T = {
+ (sessionConf) match {
+ case Some(sessionConf) =>
+ (proxyUser) match {
+ case Some(user) =>
+ Utils.doAs(user) { () =>
+ var yarnClient: YarnClient = null
+ try {
+ val yarnConf = KyuubiHadoopUtils.newYarnConfiguration(sessionConf)
+ yarnClient = createYarnClient(yarnConf)
+ action(yarnClient)
+ } finally {
+ Utils.tryLogNonFatalError(yarnClient.close())
+ }
+ }
+ case None =>
+ var yarnClient: YarnClient = null
+ try {
+ yarnClient = createAdminYarnClient(sessionConf).get
+ action(yarnClient)
+ } finally {
+ Utils.tryLogNonFatalError(yarnClient.close())
+ }
+ }
+ case None =>
+ withYarnClient(proxyUser)(action)
+ }
+ }
+
+ def createAdminYarnClient(conf: KyuubiConf): Option[YarnClient] = {
+
+ val yarnConf = KyuubiHadoopUtils.newYarnConfiguration(conf)
+
+ def createYarnClientWithCurrentUser(): Option[YarnClient] = {
+ val c = createYarnClient(yarnConf)
+ info(s"Creating admin YARN client with current user: ${Utils.currentUser}.")
+ Some(c);
+ }
+
+ def createYarnClientWithProxyUser(proxyUser: String): Option[YarnClient] =
+ Utils.doAs(proxyUser) { () =>
+ val c = createYarnClient(yarnConf)
+ info(s"Creating admin YARN client with proxy user: $proxyUser.")
+ Some(c);
+ }
+
+ YarnUserStrategy.withName(conf.get(KyuubiConf.YARN_USER_STRATEGY)) match {
+ case NONE =>
+ createYarnClientWithCurrentUser()
+ case ADMIN if conf.get(KyuubiConf.YARN_USER_ADMIN) == Utils.currentUser =>
+ createYarnClientWithCurrentUser()
+ case ADMIN =>
+ createYarnClientWithProxyUser(conf.get(KyuubiConf.YARN_USER_ADMIN))
+ case OWNER =>
+ info("Skip initializing admin YARN client")
+ None
+ }
+ }
+
override def isSupported(appMgrInfo: ApplicationManagerInfo): Boolean =
appMgrInfo.resourceManager.exists(_.toLowerCase(Locale.ROOT).startsWith("yarn"))
override def killApplicationByTag(
+ sessionConf: Option[KyuubiConf],
appMgrInfo: ApplicationManagerInfo,
tag: String,
- proxyUser: Option[String] = None): KillResponse = withYarnClient(proxyUser) { yarnClient =>
- try {
- val reports = yarnClient.getApplications(null, null, Set(tag).asJava)
- if (reports.isEmpty) {
- (false, NOT_FOUND)
- } else {
- try {
- val applicationId = reports.get(0).getApplicationId
- yarnClient.killApplication(applicationId)
- (true, s"Succeeded to terminate: $applicationId with $tag")
- } catch {
- case e: Exception =>
- (false, s"Failed to terminate application with $tag, due to ${e.getMessage}")
+ proxyUser: Option[String] = None): KillResponse =
+ withNewYarnClient(proxyUser, sessionConf) { yarnClient =>
Review Comment:
It seems that `newYarnClient` is not always needed. Can we use it only when needed? Like, the `hadoop_conf_dir/yarn_conf_dir` engine evn exists in `sessionConf`.
--
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 #6358] Fix the incorrect construction of yarnConf [kyuubi]
Posted by "ic4y (via GitHub)" <gi...@apache.org>.
ic4y commented on code in PR #6386:
URL: https://github.com/apache/kyuubi/pull/6386#discussion_r1597399291
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/YarnApplicationOperation.scala:
##########
@@ -95,74 +95,139 @@ class YarnApplicationOperation extends ApplicationOperation with Logging {
}
}
+ private def withNewYarnClient[T](
+ proxyUser: Option[String],
+ sessionConf: Option[KyuubiConf])(action: YarnClient => T): T = {
+ (sessionConf) match {
+ case Some(sessionConf) =>
+ (proxyUser) match {
+ case Some(user) =>
+ Utils.doAs(user) { () =>
+ var yarnClient: YarnClient = null
+ try {
+ val yarnConf = KyuubiHadoopUtils.newYarnConfiguration(sessionConf)
+ yarnClient = createYarnClient(yarnConf)
+ action(yarnClient)
+ } finally {
+ Utils.tryLogNonFatalError(yarnClient.close())
+ }
+ }
+ case None =>
+ var yarnClient: YarnClient = null
+ try {
+ yarnClient = createAdminYarnClient(sessionConf).get
+ action(yarnClient)
+ } finally {
+ Utils.tryLogNonFatalError(yarnClient.close())
+ }
+ }
+ case None =>
+ withYarnClient(proxyUser)(action)
+ }
+ }
+
+ def createAdminYarnClient(conf: KyuubiConf): Option[YarnClient] = {
+
+ val yarnConf = KyuubiHadoopUtils.newYarnConfiguration(conf)
+
+ def createYarnClientWithCurrentUser(): Option[YarnClient] = {
+ val c = createYarnClient(yarnConf)
+ info(s"Creating admin YARN client with current user: ${Utils.currentUser}.")
+ Some(c);
+ }
+
+ def createYarnClientWithProxyUser(proxyUser: String): Option[YarnClient] =
+ Utils.doAs(proxyUser) { () =>
+ val c = createYarnClient(yarnConf)
+ info(s"Creating admin YARN client with proxy user: $proxyUser.")
+ Some(c);
+ }
+
+ YarnUserStrategy.withName(conf.get(KyuubiConf.YARN_USER_STRATEGY)) match {
+ case NONE =>
+ createYarnClientWithCurrentUser()
+ case ADMIN if conf.get(KyuubiConf.YARN_USER_ADMIN) == Utils.currentUser =>
+ createYarnClientWithCurrentUser()
+ case ADMIN =>
+ createYarnClientWithProxyUser(conf.get(KyuubiConf.YARN_USER_ADMIN))
+ case OWNER =>
+ info("Skip initializing admin YARN client")
+ None
+ }
+ }
+
override def isSupported(appMgrInfo: ApplicationManagerInfo): Boolean =
appMgrInfo.resourceManager.exists(_.toLowerCase(Locale.ROOT).startsWith("yarn"))
override def killApplicationByTag(
+ sessionConf: Option[KyuubiConf],
appMgrInfo: ApplicationManagerInfo,
tag: String,
- proxyUser: Option[String] = None): KillResponse = withYarnClient(proxyUser) { yarnClient =>
- try {
- val reports = yarnClient.getApplications(null, null, Set(tag).asJava)
- if (reports.isEmpty) {
- (false, NOT_FOUND)
- } else {
- try {
- val applicationId = reports.get(0).getApplicationId
- yarnClient.killApplication(applicationId)
- (true, s"Succeeded to terminate: $applicationId with $tag")
- } catch {
- case e: Exception =>
- (false, s"Failed to terminate application with $tag, due to ${e.getMessage}")
+ proxyUser: Option[String] = None): KillResponse =
+ withNewYarnClient(proxyUser, sessionConf) { yarnClient =>
Review Comment:
Thanks for your help, you're right, I'll make the changes.
--
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 #6358] Fix the incorrect construction of yarnConf [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #6386:
URL: https://github.com/apache/kyuubi/pull/6386#discussion_r1597398405
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ApplicationOperation.scala:
##########
@@ -56,6 +56,7 @@ trait ApplicationOperation {
* @note For implementations, please suppress exceptions and always return KillResponse
*/
def killApplicationByTag(
+ sessionConf: Option[KyuubiConf],
Review Comment:
```suggestion
def killApplicationByTag(
appMgrInfo: ApplicationManagerInfo,
tag: String,
proxyUser: Option[String] = None,
sessionConf: Option[KyuubiConf] = None): KillResponse
```
--
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 #6358] Fix the incorrect construction of yarnConf [kyuubi]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #6386:
URL: https://github.com/apache/kyuubi/pull/6386#discussion_r1597399525
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/YarnApplicationOperation.scala:
##########
@@ -95,74 +95,139 @@ class YarnApplicationOperation extends ApplicationOperation with Logging {
}
}
+ private def withNewYarnClient[T](
+ proxyUser: Option[String],
+ sessionConf: Option[KyuubiConf])(action: YarnClient => T): T = {
+ (sessionConf) match {
+ case Some(sessionConf) =>
+ (proxyUser) match {
+ case Some(user) =>
+ Utils.doAs(user) { () =>
+ var yarnClient: YarnClient = null
+ try {
+ val yarnConf = KyuubiHadoopUtils.newYarnConfiguration(sessionConf)
Review Comment:
Does this use `kyuubi.engineEnv.HADOOP_CONF_DIR/YARN_CONF_DIR` in sessionConf to initialize `yarnConf`?
--
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