You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/04/10 09:41:02 UTC

[GitHub] [incubator-kyuubi] Nick-0723 opened a new pull request, #2314: [KYUUBI #2289] Use unique tag to kill applications

Nick-0723 opened a new pull request, #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314

   <!--
   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/contributions.html
     2. If the PR is related to an issue in https://github.com/apache/incubator-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.
   -->
   Use unique tag to kill applications instaed of log
   #2289 
   
   ### _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
   
   - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
   


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

[GitHub] [incubator-kyuubi] ulysses-you commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851866810


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala:
##########
@@ -185,6 +186,12 @@ trait ProcBuilder {
     process
   }
 
+  def killApplicationWrap(engineRefId: String): String =

Review Comment:
   I have read this history.. just think it's unnecessary



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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r849635584


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")

Review Comment:
   thanks， I saw your pr is drafted，I’ll add it to this pr



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

[GitHub] [incubator-kyuubi] turboFei commented on pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#issuecomment-1100571269

   left some comment, LGTM overall


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

[GitHub] [incubator-kyuubi] Nick-0723 commented on pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#issuecomment-1098874440

   seems flaky test failures, can you help? @yaooqinn 
   
   ```
   - hive process builder *** FAILED ***
     org.apache.kyuubi.KyuubiSQLException: JAVA_HOME is not set! For more information on installing and configuring JAVA_HOME, please visit https://kyuubi.apache.org/docs/latest/deployment/settings.html#environments
     at org.apache.kyuubi.KyuubiSQLException$.apply(KyuubiSQLException.scala:69)
     at org.apache.kyuubi.engine.ProcBuilder.validateEnv(ProcBuilder.scala:326)
     at org.apache.kyuubi.engine.ProcBuilder.validateEnv$(ProcBuilder.scala:325)
     at org.apache.kyuubi.engine.hive.HiveProcessBuilder.validateEnv(HiveProcessBuilder.scala:33)
     at org.apache.kyuubi.engine.ProcBuilder.executable(ProcBuilder.scala:52)
     at org.apache.kyuubi.engine.ProcBuilder.executable$(ProcBuilder.scala:49)
     at org.apache.kyuubi.engine.hive.HiveProcessBuilder.executable(HiveProcessBuilder.scala:33)
     at org.apache.kyuubi.engine.hive.HiveProcessBuilder.commands(HiveProcessBuilder.scala:47)
     at org.apache.kyuubi.engine.hive.HiveProcessBuilder.toString(HiveProcessBuilder.scala:89)
     at org.apache.kyuubi.engine.hive.HiveProcessBuilderSuite.$anonfun$new$1(HiveProcessBuilderSuite.scala:29)
   ```


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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r846979895


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala:
##########
@@ -240,21 +240,22 @@ class SparkProcessBuilderSuite extends KerberizedTestHelper with MockitoSugar {
   }
 
   test("kill application") {
+    val engineRefId = "cf51ba6d-b019-4f8a-a2a2-4e479aa95112"
     val pb1 = new FakeSparkProcessBuilder(conf) {
       override protected def env: Map[String, String] = Map()
       override def getYarnClient: YarnClient = mock[YarnClient]
     }
-    val exit1 = pb1.killApplication("21/09/30 17:12:47 INFO yarn.Client: " +
-      "Application report for application_1593587619692_20149 (state: ACCEPTED)")
-    assert(exit1.contains("Killed Application application_1593587619692_20149 successfully."))
+    pb1.conf.set("spark.master", "yarn")
+    val exit1 = pb1.killApplication(engineRefId)
+    assert(exit1.equals(s"There are no Application tagged with $engineRefId"))
 

Review Comment:
   Actually I don't know how to add yarn test cases



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

[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r846968348


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")

Review Comment:
   nit: don't need to be `local`



##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")
+    if (master.equals("yarn")) {
+      var applicationId: ApplicationId = null
+      try {
+        val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
+        yarnClient.init(hadoopConf)
+        yarnClient.start()
+        val apps = yarnClient.getApplications(null, null, Set(engineRefId).asJava)
+        if (apps.isEmpty) return s"There are no Application tagged with $engineRefId"
+        applicationId = apps.asScala.head.getApplicationId
+        yarnClient.killApplication(applicationId)
+        s"Killed Application $applicationId successfully."
+      } catch {
+        case e: Throwable =>
+          s"Failed to kill Application $applicationId tagged with $engineRefId," +
+            s" please kill it manually. Caused by ${e.getMessage}."
+      } finally {
+        if (yarnClient != null) {

Review Comment:
   yarn client nevel null



##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {

Review Comment:
   YARN_APP_NAME_REGEX is not used anymore



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

[GitHub] [incubator-kyuubi] yaooqinn closed pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
yaooqinn closed pull request #2314: [KYUUBI #2289] Use unique tag to kill applications 
URL: https://github.com/apache/incubator-kyuubi/pull/2314


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

[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r849550251


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")

Review Comment:
   FYI:
   ```
     private def getSparkDefaultsConf(): Map[String, String] = {
       val sparkDefaultsConfFile = env.get("SPARK_CONF_DIR")
         .orElse(env.get("SPARK_HOME").map(_ + "/conf"))
         .map(_ + "/spark-defaults.conf").map(new File(_))
         .filter(_.exists())
       Utils.getPropertiesFromFile(sparkDefaultsConfFile)
     }
   
   conf.getOption("spark.master").getOrElse(getSparkDefaultsConf().get("spark.master"))
   ```



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

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r846965876


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala:
##########
@@ -185,6 +186,12 @@ trait ProcBuilder {
     process
   }
 
+  def killApplicationWrap(engineRefId: String): String =

Review Comment:
   I have an alternative idea for it, change the method signature to `def killApplication(clue: Either[String, String]): String`, WDYT @Nick-0723 @yaooqinn?



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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r847796229


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")

Review Comment:
   Yes, we can't get the configuration in spark-defaults.conf, or is there a way I don't know?
   cc @yaooqinn 



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

[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851587343


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderOnYarnSuite.scala:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.spark
+
+import java.util.UUID
+import java.util.concurrent.TimeUnit
+
+import org.apache.hadoop.yarn.client.api.YarnClient
+import org.scalatestplus.mockito.MockitoSugar.mock
+
+import org.apache.kyuubi.{Utils, WithKyuubiServerOnYarn}
+import org.apache.kyuubi.config.KyuubiConf
+
+class SparkProcessBuilderOnYarnSuite extends WithKyuubiServerOnYarn {
+
+  override protected val kyuubiServerConf: KyuubiConf = KyuubiConf()
+
+  override protected val connectionConf: Map[String, String] = Map(
+    "spark.master" -> "yarn",
+    "spark.executor.instances" -> "1")
+
+  test("test kill application") {
+    val engineRefId = UUID.randomUUID().toString
+
+    conf.set(
+      SparkProcessBuilder.TAG_KEY,
+      conf.getOption(SparkProcessBuilder.TAG_KEY).map(_ + ",").getOrElse("") +
+        "KYUUBI," + engineRefId)
+    val builder = new SparkProcessBuilder(Utils.currentUser, conf)
+    val proc = builder.start
+    proc.waitFor(30, TimeUnit.SECONDS)

Review Comment:
   I saw that
   ```
     val yarnClient = getYarnClient
   ```
   is only used in method `killApplicationByTag`.
   
   So, I think you can make `yarnClient` as an temporary variable in `killApplicationByTag`.



##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderOnYarnSuite.scala:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.spark
+
+import java.util.UUID
+import java.util.concurrent.TimeUnit
+
+import org.apache.hadoop.yarn.client.api.YarnClient
+import org.scalatestplus.mockito.MockitoSugar.mock
+
+import org.apache.kyuubi.{Utils, WithKyuubiServerOnYarn}
+import org.apache.kyuubi.config.KyuubiConf
+
+class SparkProcessBuilderOnYarnSuite extends WithKyuubiServerOnYarn {
+
+  override protected val kyuubiServerConf: KyuubiConf = KyuubiConf()
+
+  override protected val connectionConf: Map[String, String] = Map(
+    "spark.master" -> "yarn",
+    "spark.executor.instances" -> "1")
+
+  test("test kill application") {
+    val engineRefId = UUID.randomUUID().toString
+
+    conf.set(
+      SparkProcessBuilder.TAG_KEY,
+      conf.getOption(SparkProcessBuilder.TAG_KEY).map(_ + ",").getOrElse("") +
+        "KYUUBI," + engineRefId)
+    val builder = new SparkProcessBuilder(Utils.currentUser, conf)
+    val proc = builder.start
+    proc.waitFor(30, TimeUnit.SECONDS)

Review Comment:
   I saw that
   ```
     val yarnClient = getYarnClient
   ```
   is only used in method `killApplicationByTag`.
   
   So, I think you can make `yarnClient` as a temporary variable in `killApplicationByTag`.



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

[GitHub] [incubator-kyuubi] zwangsheng commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
zwangsheng commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r846961863


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")
+    if (master.equals("yarn")) {
+      var applicationId: ApplicationId = null
+      try {
+        val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
+        yarnClient.init(hadoopConf)
+        yarnClient.start()
+        val apps = yarnClient.getApplications(null, null, Set(engineRefId).asJava)
+        if (apps.isEmpty) return s"There are no Application tagged with $engineRefId"
+        applicationId = apps.asScala.head.getApplicationId
+        yarnClient.killApplication(applicationId)
+        s"Killed Application $applicationId successfully."
+      } catch {
+        case e: Throwable =>
+          s"Failed to kill Application $applicationId tagged with $engineRefId," +
+            s" please kill it manually. Caused by ${e.getMessage}."
+      } finally {
+        if (yarnClient != null) {
+          yarnClient.stop()
         }
-      case None => ""
+      }
+    } else {
+      ""

Review Comment:
   How about we add a warning here to tell that killApplication currently only works with YARN ?



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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851583922


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -78,7 +78,12 @@ class FlinkProcessBuilder(
 
   override protected def commands: Array[String] = Array(executable)
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String = {
+  override def killApplication(clue: Either[String, String]): String = clue match {
+    case Left(_) => ""

Review Comment:
   Kent said to deal with Spark first, should Flink be put in another pr?



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

[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851587759


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderOnYarnSuite.scala:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.spark
+
+import java.util.UUID
+import java.util.concurrent.TimeUnit
+
+import org.apache.hadoop.yarn.client.api.YarnClient
+import org.scalatestplus.mockito.MockitoSugar.mock
+
+import org.apache.kyuubi.{Utils, WithKyuubiServerOnYarn}
+import org.apache.kyuubi.config.KyuubiConf
+
+class SparkProcessBuilderOnYarnSuite extends WithKyuubiServerOnYarn {
+
+  override protected val kyuubiServerConf: KyuubiConf = KyuubiConf()
+
+  override protected val connectionConf: Map[String, String] = Map(
+    "spark.master" -> "yarn",
+    "spark.executor.instances" -> "1")
+
+  test("test kill application") {
+    val engineRefId = UUID.randomUUID().toString
+
+    conf.set(
+      SparkProcessBuilder.TAG_KEY,
+      conf.getOption(SparkProcessBuilder.TAG_KEY).map(_ + ",").getOrElse("") +
+        "KYUUBI," + engineRefId)
+    val builder = new SparkProcessBuilder(Utils.currentUser, conf)
+    val proc = builder.start
+    proc.waitFor(30, TimeUnit.SECONDS)

Review Comment:
   We do not need to use singleton I think.



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

[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851586189


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderOnYarnSuite.scala:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.spark
+
+import java.util.UUID
+import java.util.concurrent.TimeUnit
+
+import org.apache.hadoop.yarn.client.api.YarnClient
+import org.scalatestplus.mockito.MockitoSugar.mock
+
+import org.apache.kyuubi.{Utils, WithKyuubiServerOnYarn}
+import org.apache.kyuubi.config.KyuubiConf
+
+class SparkProcessBuilderOnYarnSuite extends WithKyuubiServerOnYarn {
+
+  override protected val kyuubiServerConf: KyuubiConf = KyuubiConf()
+
+  override protected val connectionConf: Map[String, String] = Map(
+    "spark.master" -> "yarn",
+    "spark.executor.instances" -> "1")
+
+  test("test kill application") {
+    val engineRefId = UUID.randomUUID().toString
+
+    conf.set(
+      SparkProcessBuilder.TAG_KEY,
+      conf.getOption(SparkProcessBuilder.TAG_KEY).map(_ + ",").getOrElse("") +
+        "KYUUBI," + engineRefId)
+    val builder = new SparkProcessBuilder(Utils.currentUser, conf)
+    val proc = builder.start
+    proc.waitFor(30, TimeUnit.SECONDS)

Review Comment:
   nit: is `eventually` better?



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

[GitHub] [incubator-kyuubi] ulysses-you commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851854469


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -78,7 +78,12 @@ class FlinkProcessBuilder(
 
   override protected def commands: Array[String] = Array(executable)
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String = {
+  override def killApplication(clue: Either[String, String]): String = clue match {

Review Comment:
   And override the `killApplication` in `SparkProcessBuilder`.  BTW, we can rename the input parameter of `killApplication` to engineRefId



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

[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r847234271


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala:
##########
@@ -240,21 +240,22 @@ class SparkProcessBuilderSuite extends KerberizedTestHelper with MockitoSugar {
   }
 
   test("kill application") {
+    val engineRefId = "cf51ba6d-b019-4f8a-a2a2-4e479aa95112"
     val pb1 = new FakeSparkProcessBuilder(conf) {
       override protected def env: Map[String, String] = Map()
       override def getYarnClient: YarnClient = mock[YarnClient]
     }
-    val exit1 = pb1.killApplication("21/09/30 17:12:47 INFO yarn.Client: " +
-      "Application report for application_1593587619692_20149 (state: ACCEPTED)")
-    assert(exit1.contains("Killed Application application_1593587619692_20149 successfully."))
+    pb1.conf.set("spark.master", "yarn")
+    val exit1 = pb1.killApplication(engineRefId)
+    assert(exit1.equals(s"There are no Application tagged with $engineRefId"))
 

Review Comment:
   FYI: org.apache.kyuubi.server.MiniYarnService



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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r847360328


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala:
##########
@@ -240,21 +240,22 @@ class SparkProcessBuilderSuite extends KerberizedTestHelper with MockitoSugar {
   }
 
   test("kill application") {
+    val engineRefId = "cf51ba6d-b019-4f8a-a2a2-4e479aa95112"
     val pb1 = new FakeSparkProcessBuilder(conf) {
       override protected def env: Map[String, String] = Map()
       override def getYarnClient: YarnClient = mock[YarnClient]
     }
-    val exit1 = pb1.killApplication("21/09/30 17:12:47 INFO yarn.Client: " +
-      "Application report for application_1593587619692_20149 (state: ACCEPTED)")
-    assert(exit1.contains("Killed Application application_1593587619692_20149 successfully."))
+    pb1.conf.set("spark.master", "yarn")
+    val exit1 = pb1.killApplication(engineRefId)
+    assert(exit1.equals(s"There are no Application tagged with $engineRefId"))
 

Review Comment:
   > FYI: org.apache.kyuubi.server.MiniYarnService
   
   thanks



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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851587961


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderOnYarnSuite.scala:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.spark
+
+import java.util.UUID
+import java.util.concurrent.TimeUnit
+
+import org.apache.hadoop.yarn.client.api.YarnClient
+import org.scalatestplus.mockito.MockitoSugar.mock
+
+import org.apache.kyuubi.{Utils, WithKyuubiServerOnYarn}
+import org.apache.kyuubi.config.KyuubiConf
+
+class SparkProcessBuilderOnYarnSuite extends WithKyuubiServerOnYarn {
+
+  override protected val kyuubiServerConf: KyuubiConf = KyuubiConf()
+
+  override protected val connectionConf: Map[String, String] = Map(
+    "spark.master" -> "yarn",
+    "spark.executor.instances" -> "1")
+
+  test("test kill application") {
+    val engineRefId = UUID.randomUUID().toString
+
+    conf.set(
+      SparkProcessBuilder.TAG_KEY,
+      conf.getOption(SparkProcessBuilder.TAG_KEY).map(_ + ",").getOrElse("") +
+        "KYUUBI," + engineRefId)
+    val builder = new SparkProcessBuilder(Utils.currentUser, conf)
+    val proc = builder.start
+    proc.waitFor(30, TimeUnit.SECONDS)

Review Comment:
   ok, tks



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

[GitHub] [incubator-kyuubi] ulysses-you commented on pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#issuecomment-1097871426

   there are some conflicts


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

[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851584322


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -78,7 +78,12 @@ class FlinkProcessBuilder(
 
   override protected def commands: Array[String] = Array(executable)
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String = {
+  override def killApplication(clue: Either[String, String]): String = clue match {
+    case Left(_) => ""

Review Comment:
   ok， we can do that in another pr



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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851587720


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderOnYarnSuite.scala:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.spark
+
+import java.util.UUID
+import java.util.concurrent.TimeUnit
+
+import org.apache.hadoop.yarn.client.api.YarnClient
+import org.scalatestplus.mockito.MockitoSugar.mock
+
+import org.apache.kyuubi.{Utils, WithKyuubiServerOnYarn}
+import org.apache.kyuubi.config.KyuubiConf
+
+class SparkProcessBuilderOnYarnSuite extends WithKyuubiServerOnYarn {
+
+  override protected val kyuubiServerConf: KyuubiConf = KyuubiConf()
+
+  override protected val connectionConf: Map[String, String] = Map(
+    "spark.master" -> "yarn",
+    "spark.executor.instances" -> "1")
+
+  test("test kill application") {
+    val engineRefId = UUID.randomUUID().toString
+
+    conf.set(
+      SparkProcessBuilder.TAG_KEY,
+      conf.getOption(SparkProcessBuilder.TAG_KEY).map(_ + ",").getOrElse("") +
+        "KYUUBI," + engineRefId)
+    val builder = new SparkProcessBuilder(Utils.currentUser, conf)
+    val proc = builder.start
+    proc.waitFor(30, TimeUnit.SECONDS)

Review Comment:
   I’m not sure we need to use singleton 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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851866032


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala:
##########
@@ -185,6 +186,12 @@ trait ProcBuilder {
     process
   }
 
+  def killApplicationWrap(engineRefId: String): String =

Review Comment:
   @ulysses-you , I think this conversation will solve your question



##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala:
##########
@@ -185,6 +186,12 @@ trait ProcBuilder {
     process
   }
 
+  def killApplicationWrap(engineRefId: String): String =

Review Comment:
   @ulysses-you , I think this conversation will solve your question



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

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851940235


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -78,7 +78,12 @@ class FlinkProcessBuilder(
 
   override protected def commands: Array[String] = Array(executable)
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String = {
+  override def killApplication(clue: Either[String, String]): String = clue match {

Review Comment:
   > ... and others call `killApplication()`
   
   Then it is not testable



##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -78,7 +78,12 @@ class FlinkProcessBuilder(
 
   override protected def commands: Array[String] = Array(executable)
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String = {
+  override def killApplication(clue: Either[String, String]): String = clue match {

Review Comment:
   > ... and others call `killApplication()`
   
   Then it is not testable



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

[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#issuecomment-1101970602

   thanks, merged to master


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

[GitHub] [incubator-kyuubi] Nick-0723 commented on pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#issuecomment-1098947067

   > > HiveProcessBuilderSuite
   > 
   > Can you make a follow up get getting JAVA_HOME with falling back to `System.getProperty("java.home")`
   
   I can't reproduce this error locally
   
   <img width="941" alt="image" src="https://user-images.githubusercontent.com/31242104/163359485-449c60f8-b9c0-430f-b200-9c40c48630d9.png">
   


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

[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r848009320


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")

Review Comment:
   We can get spark.master conf here; or else from spark-defaults.conf from "SPARK_CONF_DIR"; or else SPARK_HOME/conf



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

[GitHub] [incubator-kyuubi] cxzl25 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
cxzl25 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r847686508


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")

Review Comment:
   In this way, `spark.master` can only be configured in `kyuubi-defaults.conf` to support automatic kill yarn app, but not in `spark-defaults.conf`?
   
   nit:`spark.master` can be used as a constant of `object SparkProcessBuilder`.



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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r846979290


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala:
##########
@@ -185,6 +186,12 @@ trait ProcBuilder {
     process
   }
 
+  def killApplicationWrap(engineRefId: String): String =

Review Comment:
   So we judge whether it is Left or Right in EngineRef? We can't get logs in EngineRef



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

[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r847011038


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala:
##########
@@ -185,6 +186,12 @@ trait ProcBuilder {
     process
   }
 
+  def killApplicationWrap(engineRefId: String): String =

Review Comment:
   the right will be removed eventually



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

[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r847234271


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala:
##########
@@ -240,21 +240,22 @@ class SparkProcessBuilderSuite extends KerberizedTestHelper with MockitoSugar {
   }
 
   test("kill application") {
+    val engineRefId = "cf51ba6d-b019-4f8a-a2a2-4e479aa95112"
     val pb1 = new FakeSparkProcessBuilder(conf) {
       override protected def env: Map[String, String] = Map()
       override def getYarnClient: YarnClient = mock[YarnClient]
     }
-    val exit1 = pb1.killApplication("21/09/30 17:12:47 INFO yarn.Client: " +
-      "Application report for application_1593587619692_20149 (state: ACCEPTED)")
-    assert(exit1.contains("Killed Application application_1593587619692_20149 successfully."))
+    pb1.conf.set("spark.master", "yarn")
+    val exit1 = pb1.killApplication(engineRefId)
+    assert(exit1.equals(s"There are no Application tagged with $engineRefId"))
 

Review Comment:
   FYI: MiniYarnService



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

[GitHub] [incubator-kyuubi] zwangsheng commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
zwangsheng commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r846965649


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderSuite.scala:
##########
@@ -240,21 +240,22 @@ class SparkProcessBuilderSuite extends KerberizedTestHelper with MockitoSugar {
   }
 
   test("kill application") {
+    val engineRefId = "cf51ba6d-b019-4f8a-a2a2-4e479aa95112"
     val pb1 = new FakeSparkProcessBuilder(conf) {
       override protected def env: Map[String, String] = Map()
       override def getYarnClient: YarnClient = mock[YarnClient]
     }
-    val exit1 = pb1.killApplication("21/09/30 17:12:47 INFO yarn.Client: " +
-      "Application report for application_1593587619692_20149 (state: ACCEPTED)")
-    assert(exit1.contains("Killed Application application_1593587619692_20149 successfully."))
+    pb1.conf.set("spark.master", "yarn")
+    val exit1 = pb1.killApplication(engineRefId)
+    assert(exit1.equals(s"There are no Application tagged with $engineRefId"))
 

Review Comment:
   If possible, add a successful kill application case 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

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r847003148


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala:
##########
@@ -185,6 +186,12 @@ trait ProcBuilder {
     process
   }
 
+  def killApplicationWrap(engineRefId: String): String =

Review Comment:
   Yeah, I mean merge two methods to one. And use `Left` to represent  `engineRefId` and Right to represent `line`



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

[GitHub] [incubator-kyuubi] ulysses-you commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851853845


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -78,7 +78,12 @@ class FlinkProcessBuilder(
 
   override protected def commands: Array[String] = Array(executable)
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String = {
+  override def killApplication(clue: Either[String, String]): String = clue match {

Review Comment:
   Why do we need this change ? For Spark SQL engine, we can call `killApplication(engineRefId)` and others call `killApplication()`



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

[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851581636


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/flink/FlinkProcessBuilder.scala:
##########
@@ -78,7 +78,12 @@ class FlinkProcessBuilder(
 
   override protected def commands: Array[String] = Array(executable)
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String = {
+  override def killApplication(clue: Either[String, String]): String = clue match {
+    case Left(_) => ""

Review Comment:
   is it difficult to add engine tag for Flink engine?



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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r851586382


##########
kyuubi-server/src/test/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilderOnYarnSuite.scala:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.engine.spark
+
+import java.util.UUID
+import java.util.concurrent.TimeUnit
+
+import org.apache.hadoop.yarn.client.api.YarnClient
+import org.scalatestplus.mockito.MockitoSugar.mock
+
+import org.apache.kyuubi.{Utils, WithKyuubiServerOnYarn}
+import org.apache.kyuubi.config.KyuubiConf
+
+class SparkProcessBuilderOnYarnSuite extends WithKyuubiServerOnYarn {
+
+  override protected val kyuubiServerConf: KyuubiConf = KyuubiConf()
+
+  override protected val connectionConf: Map[String, String] = Map(
+    "spark.master" -> "yarn",
+    "spark.executor.instances" -> "1")
+
+  test("test kill application") {
+    val engineRefId = UUID.randomUUID().toString
+
+    conf.set(
+      SparkProcessBuilder.TAG_KEY,
+      conf.getOption(SparkProcessBuilder.TAG_KEY).map(_ + ",").getOrElse("") +
+        "KYUUBI," + engineRefId)
+    val builder = new SparkProcessBuilder(Utils.currentUser, conf)
+    val proc = builder.start
+    proc.waitFor(30, TimeUnit.SECONDS)

Review Comment:
   `yarnClient` closes after `killApplication` is called once, I tried `eventually`



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

[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#issuecomment-1098919142

   > HiveProcessBuilderSuite
   
   Can you make a follow up get getting JAVA_HOME with falling back to `System.getProperty("java.home")`


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

[GitHub] [incubator-kyuubi] Nick-0723 commented on pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#issuecomment-1100693045

   If you have time, please help review this. @yaooqinn 


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

[GitHub] [incubator-kyuubi] turboFei commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
turboFei commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r849550251


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")

Review Comment:
   FYI:
   ```
     private def getSparkDefaultsConf(): Map[String, String] = {
       val sparkDefaultsConfFile = env.get("SPARK_CONF_DIR")
         .orElse(env.get("SPARK_HOME").map(_ + "/conf"))
         .map(_ + "/spark-defaults.conf").map(new File(_))
         .filter(_.exists())
       Utils.getPropertiesFromFile(sparkDefaultsConfFile)
     }
   
   conf.getOption("spark.master").getOrElse(getSparkDefaultsConf().get("spark.master"))
   ```
   
   You can use CONSTANT to replace above hard coded string value.



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

[GitHub] [incubator-kyuubi] Nick-0723 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
Nick-0723 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r847015388


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala:
##########
@@ -185,6 +186,12 @@ trait ProcBuilder {
     process
   }
 
+  def killApplicationWrap(engineRefId: String): String =

Review Comment:
   > Yeah, I mean merge two methods to one. And use `Left` to represent `engineRefId` and Right to represent `line`
   
   If it is merged into one, the implementation of the flink engine will also needs to be 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

[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#issuecomment-1094238460

   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#2314](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9877625) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/82a024a9837104bb74d6ea9ba103439794b84232?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (82a024a) will **increase** coverage by `0.01%`.
   > The diff coverage is `68.18%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #2314      +/-   ##
   ============================================
   + Coverage     62.25%   62.27%   +0.01%     
     Complexity       69       69              
   ============================================
     Files           353      353              
     Lines         16757    16807      +50     
     Branches       2269     2276       +7     
   ============================================
   + Hits          10432    10466      +34     
   - Misses         5366     5374       +8     
   - Partials        959      967       +8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvUHJvY0J1aWxkZXIuc2NhbGE=) | `83.47% <0.00%> (-2.13%)` | :arrow_down: |
   | [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvRW5naW5lUmVmLnNjYWxh) | `75.96% <66.66%> (+0.23%)` | :arrow_up: |
   | [...ache/kyuubi/engine/spark/SparkProcessBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvc3BhcmsvU3BhcmtQcm9jZXNzQnVpbGRlci5zY2FsYQ==) | `85.57% <81.25%> (-0.70%)` | :arrow_down: |
   | [...ain/scala/org/apache/kyuubi/util/ThreadUtils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS91dGlsL1RocmVhZFV0aWxzLnNjYWxh) | `86.20% <0.00%> (-13.80%)` | :arrow_down: |
   | [...org/apache/kyuubi/ha/client/ServiceDiscovery.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9TZXJ2aWNlRGlzY292ZXJ5LnNjYWxh) | `89.28% <0.00%> (-3.58%)` | :arrow_down: |
   | [...org/apache/kyuubi/operation/log/OperationLog.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vbG9nL09wZXJhdGlvbkxvZy5zY2FsYQ==) | `89.23% <0.00%> (-2.58%)` | :arrow_down: |
   | [...rg/apache/kyuubi/engine/trino/TrinoStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS10cmluby1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL3RyaW5vL1RyaW5vU3RhdGVtZW50LnNjYWxh) | `67.46% <0.00%> (-2.41%)` | :arrow_down: |
   | [.../org/apache/kyuubi/engine/hive/HiveSQLEngine.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1oaXZlLXNxbC1lbmdpbmUvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvZW5naW5lL2hpdmUvSGl2ZVNRTEVuZ2luZS5zY2FsYQ==) | `70.00% <0.00%> (-1.67%)` | :arrow_down: |
   | [...org/apache/kyuubi/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `82.66% <0.00%> (-0.23%)` | :arrow_down: |
   | [...a/org/apache/kyuubi/plugin/SessionConfAdvisor.java](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2t5dXViaS9wbHVnaW4vU2Vzc2lvbkNvbmZBZHZpc29yLmphdmE=) | | |
   | ... and [7 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [82a024a...9877625](https://codecov.io/gh/apache/incubator-kyuubi/pull/2314?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


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

[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r846969537


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")
+    if (master.equals("yarn")) {
+      var applicationId: ApplicationId = null
+      try {
+        val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
+        yarnClient.init(hadoopConf)
+        yarnClient.start()
+        val apps = yarnClient.getApplications(null, null, Set(engineRefId).asJava)
+        if (apps.isEmpty) return s"There are no Application tagged with $engineRefId"
+        applicationId = apps.asScala.head.getApplicationId
+        yarnClient.killApplication(applicationId)

Review Comment:
   `yarnClient.killApplication(applicationId, clue)` 
   it's better to add a kyuubi-specific clue head for the kill reason



##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala:
##########
@@ -193,28 +194,32 @@ class SparkProcessBuilder(
     }
   }
 
-  override def killApplication(line: String = lastRowsOfLog.toArray.mkString("\n")): String =
-    YARN_APP_NAME_REGEX.findFirstIn(line) match {
-      case Some(appId) =>
-        try {
-          val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
-          yarnClient.init(hadoopConf)
-          yarnClient.start()
-          val applicationId = ApplicationId.fromString(appId)
-          yarnClient.killApplication(applicationId)
-          s"Killed Application $appId successfully."
-        } catch {
-          case e: Throwable =>
-            s"Failed to kill Application $appId, please kill it manually." +
-              s" Caused by ${e.getMessage}."
-        } finally {
-          if (yarnClient != null) {
-            yarnClient.stop()
-          }
+  override def killApplication(engineRefId: String): String = {
+    val master = conf.getOption("spark.master").getOrElse("local")
+    if (master.equals("yarn")) {
+      var applicationId: ApplicationId = null
+      try {
+        val hadoopConf = KyuubiHadoopUtils.newHadoopConf(conf)
+        yarnClient.init(hadoopConf)
+        yarnClient.start()
+        val apps = yarnClient.getApplications(null, null, Set(engineRefId).asJava)
+        if (apps.isEmpty) return s"There are no Application tagged with $engineRefId"
+        applicationId = apps.asScala.head.getApplicationId
+        yarnClient.killApplication(applicationId)

Review Comment:
   `yarnClient.killApplication(applicationId, clue)` 
   it's better to add a kyuubi-specific clue here for the kill reason



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

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r847024891


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala:
##########
@@ -185,6 +186,12 @@ trait ProcBuilder {
     process
   }
 
+  def killApplicationWrap(engineRefId: String): String =

Review Comment:
   > If it is merged into one, the implementation of the flink engine will also needs to be modified
   
   It's not a big deal. caz it's internal API. And we can change it back if we can unify the flink to tag-based killing



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

[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #2314: [KYUUBI #2289] Use unique tag to kill applications

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #2314:
URL: https://github.com/apache/incubator-kyuubi/pull/2314#discussion_r847017983


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ProcBuilder.scala:
##########
@@ -185,6 +186,12 @@ trait ProcBuilder {
     process
   }
 
+  def killApplicationWrap(engineRefId: String): String =

Review Comment:
   I think we don't need such thing, the tag-based killing is promising



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