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/07/13 14:20:48 UTC

[GitHub] [incubator-kyuubi] turboFei opened a new pull request, #3063: Expose the operation state metrics

turboFei opened a new pull request, #3063:
URL: https://github.com/apache/incubator-kyuubi/pull/3063

   <!--
   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.
   -->
   
   
   ### _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
   
   - [ ] [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] turboFei commented on a diff in pull request #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {
+            killApplicationByTag(tag)
+          } else {
+            (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          }

Review Comment:
   It might cause that the new added UT failed.
   
   Because the new UI assert that the running count is 0 at the beginning.
   
   and assert the canceled count is increased after delete batch 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] turboFei commented on a diff in pull request #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {
+            killApplicationByTag(tag)
+          } else {
+            (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          }

Review Comment:
   It might cause that the new added UT failed.
   
   Because the new UI assert that the running count is 0 at the begining.



-- 
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 #3063: [KYUUBI #3046][Metrics] Add meter metrics for recording the rate of the operation state for each kyuubi operation

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -306,9 +306,7 @@ class BatchJobSubmission(
         case e: IOException => error(e.getMessage, e)
       }
 
-      MetricsSystem.tracing(_.decCount(
-        MetricRegistry.name(OPERATION_OPEN, statement.toLowerCase(Locale.getDefault))))

Review Comment:
   nice catch, removed



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {
+            killApplicationByTag(tag)
+          } else {
+            (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          }

Review Comment:
   It might cause that the new added UT failed.
   
   Because the new UI assert that the running count is 0 at the beginning.
   



-- 
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 #3063: [KYUUBI #3046][Metrics] Add meter metrics for recording the rate of the operation state for each kyuubi operation

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


##########
docs/monitor/metrics.md:
##########
@@ -79,6 +79,7 @@ Metrics Prefix | Metrics Suffix | Type | Since | Description
 `kyuubi.backend_service.fetch_result_rows_rate`  | | meter | 1.5.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `fetchResults` method that fetch result rows rate </div>
 `kyuubi.backend_service.get_primary_keys`  | | meter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `get_primary_keys` method execution time and rate </div>
 `kyuubi.backend_service.get_cross_reference`  | | meter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `get_cross_reference` method execution time and rate </div>
+`kyuubi.operation.state`   | `${operationType}`<br/>`.${state}`     | counter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'>  current count for the `${operationType}` with a particular `${state}`, e.g. `BatchJobSubmission.pending`</div>

Review Comment:
   the discussion is marked as resolved but the code part remains unchanged？



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
docs/monitor/metrics.md:
##########
@@ -79,6 +79,7 @@ Metrics Prefix | Metrics Suffix | Type | Since | Description
 `kyuubi.backend_service.fetch_result_rows_rate`  | | meter | 1.5.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `fetchResults` method that fetch result rows rate </div>
 `kyuubi.backend_service.get_primary_keys`  | | meter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `get_primary_keys` method execution time and rate </div>
 `kyuubi.backend_service.get_cross_reference`  | | meter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `get_cross_reference` method execution time and rate </div>
+`kyuubi.operation.state`   | `${operationType}`<br/>`.${state}`     | counter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'>  current count for the `${operationType}` with a particular `${state}`, e.g. `BatchJobSubmission.pending`</div>

Review Comment:
   thanks, it helps a lot.



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -65,6 +65,9 @@ class JpsApplicationOperation extends ApplicationOperation {
       val (id, _) = idAndCmd.splitAt(idAndCmd.indexOf(" "))
       try {
         s"kill -15 $id".lineStream
+        if (getEngine(tag).nonEmpty) {
+          killApplicationByTag(tag)

Review Comment:
   spark-submit will generate two processes.
   
   



-- 
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 #3063: [KYUUBI #3046][Metrics] Add meter metrics for recording the rate of the operation state for each kyuubi operation

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


##########
docs/monitor/metrics.md:
##########
@@ -79,6 +79,7 @@ Metrics Prefix | Metrics Suffix | Type | Since | Description
 `kyuubi.backend_service.fetch_result_rows_rate`  | | meter | 1.5.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `fetchResults` method that fetch result rows rate </div>
 `kyuubi.backend_service.get_primary_keys`  | | meter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `get_primary_keys` method execution time and rate </div>
 `kyuubi.backend_service.get_cross_reference`  | | meter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `get_cross_reference` method execution time and rate </div>
+`kyuubi.operation.state`   | `${operationType}`<br/>`.${state}`     | counter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'>  current count for the `${operationType}` with a particular `${state}`, e.g. `BatchJobSubmission.pending`</div>

Review Comment:
   ...resolved locally.



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
docs/monitor/metrics.md:
##########
@@ -79,6 +79,7 @@ Metrics Prefix | Metrics Suffix | Type | Since | Description
 `kyuubi.backend_service.fetch_result_rows_rate`  | | meter | 1.5.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `fetchResults` method that fetch result rows rate </div>
 `kyuubi.backend_service.get_primary_keys`  | | meter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `get_primary_keys` method execution time and rate </div>
 `kyuubi.backend_service.get_cross_reference`  | | meter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `get_cross_reference` method execution time and rate </div>
+`kyuubi.operation.state`   | `${operationType}`<br/>`.${state}`     | counter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'>  current count for the `${operationType}` with a particular `${state}`, e.g. `BatchJobSubmission.pending`</div>

Review Comment:
   you can use meters for this actually.. just like kyuubi.operation.state.$state
   
   ```
   "kyuubi.operation.state.closed" : {
         "count" : 19,
         "m15_rate" : 0.020137396109391522,
         "m1_rate" : 1.6595792398582304E-4,
         "m5_rate" : 0.003290775624396568,
         "mean_rate" : 0.00539128492410111,
         "units" : "events/second"
       },
       "kyuubi.operation.state.error" : {
         "count" : 1,
         "m15_rate" : 0.025462486603774714,
         "m1_rate" : 7.483284619160424E-15,
         "m5_rate" : 4.1270758361306424E-4,
         "mean_rate" : 5.372806924825531E-4,
         "units" : "events/second"
       },
       "kyuubi.operation.state.finished" : {
         "count" : 18,
         "m15_rate" : 0.01815503925250486,
         "m1_rate" : 1.5388408901182632E-14,
         "m5_rate" : 1.1365883986004687E-4,
         "mean_rate" : 0.005107080000702363,
         "units" : "events/second"
       },
       "kyuubi.operation.state.initialized" : {
         "count" : 19,
         "m15_rate" : 0.006237933279653059,
         "m1_rate" : 1.5785160613558252E-14,
         "m5_rate" : 1.1526188920321656E-4,
         "mean_rate" : 0.005376601901198222,
         "units" : "events/second"
       }
   ```



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/KyuubiOperation.scala:
##########
@@ -156,8 +157,11 @@ abstract class KyuubiOperation(session: Session) extends AbstractOperation(sessi
   override def shouldRunAsync: Boolean = false
 
   override def setState(newState: OperationState): Unit = {
+    MetricsSystem.tracing { ms =>
+      ms.decCount(MetricRegistry.name(OPERATION_STATE, opType, state.toString.toLowerCase))
+      ms.incCount(MetricRegistry.name(OPERATION_STATE, opType, newState.toString.toLowerCase))

Review Comment:
   yes



-- 
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 #3063: [KYUUBI #3046][Metrics] Add meter metrics for recording the rate of the operation state for each kyuubi operation

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -306,9 +306,7 @@ class BatchJobSubmission(
         case e: IOException => error(e.getMessage, e)
       }
 
-      MetricsSystem.tracing(_.decCount(
-        MetricRegistry.name(OPERATION_OPEN, statement.toLowerCase(Locale.getDefault))))

Review Comment:
   also remove the statement override in LaunchEngine: https://github.com/apache/incubator-kyuubi/pull/3077



-- 
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 #3063: [KYUUBI #3046][Metrics] Add meter metrics for recording the rate of the operation state for each kyuubi operation

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -306,9 +306,7 @@ class BatchJobSubmission(
         case e: IOException => error(e.getMessage, e)
       }
 
-      MetricsSystem.tracing(_.decCount(
-        MetricRegistry.name(OPERATION_OPEN, statement.toLowerCase(Locale.getDefault))))

Review Comment:
   nice catch, removed



-- 
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 #3063: [KYUUBI #3046][Metrics] Add meter metrics for recording the rate of the operation state for each kyuubi operation

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -306,9 +306,7 @@ class BatchJobSubmission(
         case e: IOException => error(e.getMessage, e)
       }
 
-      MetricsSystem.tracing(_.decCount(
-        MetricRegistry.name(OPERATION_OPEN, statement.toLowerCase(Locale.getDefault))))

Review Comment:
   shall we also remove the override of statement in this class ?



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
docs/monitor/metrics.md:
##########
@@ -79,6 +79,7 @@ Metrics Prefix | Metrics Suffix | Type | Since | Description
 `kyuubi.backend_service.fetch_result_rows_rate`  | | meter | 1.5.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `fetchResults` method that fetch result rows rate </div>
 `kyuubi.backend_service.get_primary_keys`  | | meter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `get_primary_keys` method execution time and rate </div>
 `kyuubi.backend_service.get_cross_reference`  | | meter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'> kyuubi backend service `get_cross_reference` method execution time and rate </div>
+`kyuubi.operation.state`   | `${operationType}`<br/>`.${state}`     | counter | 1.6.0 |<div style='width: 150pt;word-wrap: break-word;white-space: normal'>  current count for the `${operationType}` with a particular `${state}`, e.g. `BatchJobSubmission.pending`</div>

Review Comment:
   thanks, let me have a try



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/KyuubiOperation.scala:
##########
@@ -156,8 +157,11 @@ abstract class KyuubiOperation(session: Session) extends AbstractOperation(sessi
   override def shouldRunAsync: Boolean = false
 
   override def setState(newState: OperationState): Unit = {
+    MetricsSystem.tracing { ms =>
+      ms.decCount(MetricRegistry.name(OPERATION_STATE, opType, state.toString.toLowerCase))
+      ms.incCount(MetricRegistry.name(OPERATION_STATE, opType, newState.toString.toLowerCase))

Review Comment:
   if so, you need to document it clear



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/KyuubiOperation.scala:
##########
@@ -156,8 +157,11 @@ abstract class KyuubiOperation(session: Session) extends AbstractOperation(sessi
   override def shouldRunAsync: Boolean = false
 
   override def setState(newState: OperationState): Unit = {
+    MetricsSystem.tracing { ms =>
+      ms.decCount(MetricRegistry.name(OPERATION_STATE, opType, state.toString.toLowerCase))
+      ms.incCount(MetricRegistry.name(OPERATION_STATE, opType, newState.toString.toLowerCase))
+      ms.markMeter(MetricRegistry.name(OPERATION_STATE, newState.toString.toLowerCase))

Review Comment:
   what is different from L43



-- 
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 #3063: [KYUUBI #3046][Metrics] Add meter metrics for recording the rate of the operation state for each kyuubi operation

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {
+            killApplicationByTag(tag)
+          } else {
+            (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          }

Review Comment:
   have remove it from this pr.



##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {
+            killApplicationByTag(tag)
+          } else {
+            (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          }

Review Comment:
   have removed it from 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 a diff in pull request #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {
+            killApplicationByTag(tag)
+          } else {
+            (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          }

Review Comment:
   the spark-submit may generate two process. 
   
   And we might fail to kill the running batch jobs after each test in BatchResourceSuite.



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/KyuubiOperation.scala:
##########
@@ -156,8 +157,11 @@ abstract class KyuubiOperation(session: Session) extends AbstractOperation(sessi
   override def shouldRunAsync: Boolean = false
 
   override def setState(newState: OperationState): Unit = {
+    MetricsSystem.tracing { ms =>
+      ms.decCount(MetricRegistry.name(OPERATION_STATE, opType, state.toString.toLowerCase))
+      ms.incCount(MetricRegistry.name(OPERATION_STATE, opType, newState.toString.toLowerCase))
+      ms.markMeter(MetricRegistry.name(OPERATION_STATE, newState.toString.toLowerCase))

Review Comment:
   the state value is different with that of L43.



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -65,6 +65,9 @@ class JpsApplicationOperation extends ApplicationOperation {
       val (id, _) = idAndCmd.splitAt(idAndCmd.indexOf(" "))
       try {
         s"kill -15 $id".lineStream
+        if (getEngine(tag).nonEmpty) {
+          killApplicationByTag(tag)

Review Comment:
   to prevent flaky test.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {
+            killApplicationByTag(tag)
+          } else {
+            (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          }

Review Comment:
   so, what are we going to fix?



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
docs/monitor/metrics.md:
##########
@@ -89,3 +89,12 @@ Since v1.5.0, you can use the following metrics to replace:
 - `kyuubi.operation.total.ExecuteStatement`
 - `kyuubi.operation.opened.ExecuteStatement`
 - `kyuubi.operation.failed.ExecuteStatement.${errorType}`
+
+Since v1.6.0, you can use the following metrics to get the current state counter for a specified operation type:

Review Comment:
   the doc is eligible. please follow the table above. the one you added can only be an extra hint for behavior change, you don't need to do this for newly added ones.



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {
+            killApplicationByTag(tag)
+          } else {
+            (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          }

Review Comment:
   let's not hide bugs...



-- 
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 #3063: Expose the metrics with operation type and current state

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

   is it possible add some test ? 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] ulysses-you commented on a diff in pull request #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {

Review Comment:
   we should only try one time and fail if the last attempt does not succeed ?



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/KyuubiOperation.scala:
##########
@@ -156,8 +157,11 @@ abstract class KyuubiOperation(session: Session) extends AbstractOperation(sessi
   override def shouldRunAsync: Boolean = false
 
   override def setState(newState: OperationState): Unit = {
+    MetricsSystem.tracing { ms =>
+      ms.decCount(MetricRegistry.name(OPERATION_STATE, opType, state.toString.toLowerCase))
+      ms.incCount(MetricRegistry.name(OPERATION_STATE, opType, newState.toString.toLowerCase))

Review Comment:
   so the terminal states are cumulative, but the intermediate ones are not



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/KyuubiOperation.scala:
##########
@@ -156,8 +157,11 @@ abstract class KyuubiOperation(session: Session) extends AbstractOperation(sessi
   override def shouldRunAsync: Boolean = false
 
   override def setState(newState: OperationState): Unit = {
+    MetricsSystem.tracing { ms =>
+      ms.decCount(MetricRegistry.name(OPERATION_STATE, opType, state.toString.toLowerCase))
+      ms.incCount(MetricRegistry.name(OPERATION_STATE, opType, newState.toString.toLowerCase))
+      ms.markMeter(MetricRegistry.name(OPERATION_STATE, newState.toString.toLowerCase))

Review Comment:
   what is this for?



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -65,6 +65,9 @@ class JpsApplicationOperation extends ApplicationOperation {
       val (id, _) = idAndCmd.splitAt(idAndCmd.indexOf(" "))
       try {
         s"kill -15 $id".lineStream
+        if (getEngine(tag).nonEmpty) {
+          killApplicationByTag(tag)

Review Comment:
   seems the first process is transient and just for spark-submit arguments parser.
   
   So, if we try to kill, it might say that: Kill - No such process.



-- 
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 #3063: [KYUUBI #3046][Metrics] Add meter metrics for recording the rate of the operation state for each kyuubi operation

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

   thanks @yaooqinn @ulysses-you 
   
   merging 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] turboFei closed pull request #3063: [KYUUBI #3046][Metrics] Add meter metrics for recording the rate of the operation state for each kyuubi operation

Posted by GitBox <gi...@apache.org>.
turboFei closed pull request #3063: [KYUUBI #3046][Metrics] Add meter metrics for recording the rate of the operation state for each kyuubi operation
URL: https://github.com/apache/incubator-kyuubi/pull/3063


-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -65,6 +65,9 @@ class JpsApplicationOperation extends ApplicationOperation {
       val (id, _) = idAndCmd.splitAt(idAndCmd.indexOf(" "))
       try {
         s"kill -15 $id".lineStream
+        if (getEngine(tag).nonEmpty) {
+          killApplicationByTag(tag)

Review Comment:
   do you mean the first kill failed but with no exception but the second kill succeeded ? how can this happens..



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/KyuubiOperation.scala:
##########
@@ -156,8 +157,11 @@ abstract class KyuubiOperation(session: Session) extends AbstractOperation(sessi
   override def shouldRunAsync: Boolean = false
 
   override def setState(newState: OperationState): Unit = {
+    MetricsSystem.tracing { ms =>
+      ms.decCount(MetricRegistry.name(OPERATION_STATE, opType, state.toString.toLowerCase))
+      ms.incCount(MetricRegistry.name(OPERATION_STATE, opType, newState.toString.toLowerCase))
+      ms.markMeter(MetricRegistry.name(OPERATION_STATE, newState.toString.toLowerCase))

Review Comment:
   I think we can remove this one, it seems meaning less and always increasing.



##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/KyuubiOperation.scala:
##########
@@ -156,8 +157,11 @@ abstract class KyuubiOperation(session: Session) extends AbstractOperation(sessi
   override def shouldRunAsync: Boolean = false
 
   override def setState(newState: OperationState): Unit = {
+    MetricsSystem.tracing { ms =>
+      ms.decCount(MetricRegistry.name(OPERATION_STATE, opType, state.toString.toLowerCase))
+      ms.incCount(MetricRegistry.name(OPERATION_STATE, opType, newState.toString.toLowerCase))
+      ms.markMeter(MetricRegistry.name(OPERATION_STATE, newState.toString.toLowerCase))

Review Comment:
   I think we can remove this one, it seems meaning-less and always increasing.



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {
+            killApplicationByTag(tag)
+          } else {
+            (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          }

Review Comment:
   what is this for, seems not related to the scope of 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] yaooqinn commented on pull request #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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

   doc is missing


-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/KyuubiOperation.scala:
##########
@@ -156,8 +157,11 @@ abstract class KyuubiOperation(session: Session) extends AbstractOperation(sessi
   override def shouldRunAsync: Boolean = false
 
   override def setState(newState: OperationState): Unit = {
+    MetricsSystem.tracing { ms =>
+      ms.decCount(MetricRegistry.name(OPERATION_STATE, opType, state.toString.toLowerCase))
+      ms.incCount(MetricRegistry.name(OPERATION_STATE, opType, newState.toString.toLowerCase))
+      ms.markMeter(MetricRegistry.name(OPERATION_STATE, newState.toString.toLowerCase))

Review Comment:
   i see



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -65,6 +65,9 @@ class JpsApplicationOperation extends ApplicationOperation {
       val (id, _) = idAndCmd.splitAt(idAndCmd.indexOf(" "))
       try {
         s"kill -15 $id".lineStream
+        if (getEngine(tag).nonEmpty) {
+          killApplicationByTag(tag)

Review Comment:
   seems the first process is transient and just for spark-submit arguments parser.
   
   So, if we try to kill, 
   we might get the first process id, but it is transient, so if we kill the process id, it might say that: Kill - No such process.
   
   But the real process is still running, because it has another pid.



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -65,6 +65,9 @@ class JpsApplicationOperation extends ApplicationOperation {
       val (id, _) = idAndCmd.splitAt(idAndCmd.indexOf(" "))
       try {
         s"kill -15 $id".lineStream
+        if (getEngine(tag).nonEmpty) {
+          killApplicationByTag(tag)

Review Comment:
   if so the first kill will fail since there is no that process, shall we check the result of the command ? if the process is not found we should try to kill again, otherwise do nothing



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
docs/monitor/metrics.md:
##########
@@ -89,3 +89,12 @@ Since v1.5.0, you can use the following metrics to replace:
 - `kyuubi.operation.total.ExecuteStatement`
 - `kyuubi.operation.opened.ExecuteStatement`
 - `kyuubi.operation.failed.ExecuteStatement.${errorType}`
+
+Since v1.6.0, you can use the following metrics to get the current state counter for a specified operation type:

Review Comment:
   the doc is eligible. please follow the table above. the one you added can only be an extra hint.



-- 
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 #3063: Expose the metrics with operation type and current state

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

   > is it possible add some test ?
   
   sure


-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -65,6 +65,9 @@ class JpsApplicationOperation extends ApplicationOperation {
       val (id, _) = idAndCmd.splitAt(idAndCmd.indexOf(" "))
       try {
         s"kill -15 $id".lineStream
+        if (getEngine(tag).nonEmpty) {
+          killApplicationByTag(tag)

Review Comment:
   sounds good, I thought that the command will not throw exception even it failed to kill the process.
   
   Just have a testing, it will throw exception if the command fail.
   
   done.



-- 
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 #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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


##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -68,7 +68,11 @@ class JpsApplicationOperation extends ApplicationOperation {
         (true, s"Succeeded to terminate: $idAndCmd")
       } catch {
         case e: Exception =>
-          (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          if (getEngine(tag).nonEmpty) {
+            killApplicationByTag(tag)
+          } else {
+            (false, s"Failed to terminate: $idAndCmd, due to ${e.getMessage}")
+          }

Review Comment:
   yeah, we can fix this in a new 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] codecov-commenter commented on pull request #3063: [KYUUBI #3046] Expose the metrics with operation type and current state

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

   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063?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 [#3063](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f748093) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/a7d190dd7b92a5e28f6247fe91fdf3e4d7244973?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a7d190d) will **decrease** coverage by `0.00%`.
   > The diff coverage is `100.00%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3063      +/-   ##
   ============================================
   - Coverage     51.57%   51.56%   -0.01%     
     Complexity        6        6              
   ============================================
     Files           458      458              
     Lines         25254    25256       +2     
     Branches       3517     3517              
   ============================================
   - Hits          13025    13024       -1     
     Misses        10991    10991              
   - Partials       1238     1241       +3     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...g/apache/kyuubi/operation/BatchJobSubmission.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vQmF0Y2hKb2JTdWJtaXNzaW9uLnNjYWxh) | `77.77% <100.00%> (-0.14%)` | :arrow_down: |
   | [.../org/apache/kyuubi/operation/KyuubiOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vS3l1dWJpT3BlcmF0aW9uLnNjYWxh) | `70.27% <100.00%> (+1.25%)` | :arrow_up: |
   | [...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063/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-a3l1dWJpLWN0bC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jdGwvY21kL2xvZy9Mb2dCYXRjaENvbW1hbmQuc2NhbGE=) | `80.00% <0.00%> (-2.00%)` | :arrow_down: |
   | [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063/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.23% <0.00%> (-0.96%)` | :arrow_down: |
   | [...che/kyuubi/server/KyuubiTHttpFrontendService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpVEh0dHBGcm9udGVuZFNlcnZpY2Uuc2NhbGE=) | `63.97% <0.00%> (-0.74%)` | :arrow_down: |
   | [...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063/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.12% <0.00%> (-0.63%)` | :arrow_down: |
   | [...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063/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-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9ldGNkL0V0Y2REaXNjb3ZlcnlDbGllbnQuc2NhbGE=) | `73.10% <0.00%> (+0.68%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063?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/3063?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 [a7d190d...f748093](https://codecov.io/gh/apache/incubator-kyuubi/pull/3063?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