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/25 08:54:25 UTC
[GitHub] [incubator-kyuubi] ulysses-you opened a new pull request, #3137: [KYUUBI #3136] Make application info stable
ulysses-you opened a new pull request, #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137
<!--
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.
-->
closes https://github.com/apache/incubator-kyuubi/issues/3136
### _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] yaooqinn commented on a diff in pull request #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r928666830
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ApplicationOperation.scala:
##########
@@ -55,21 +56,55 @@ trait ApplicationOperation {
* Get the engine/application status by the unique application tag
*
* @param tag the unique application tag for engine instance.
- * @return a map contains the application status
+ * @return [[ApplicationInfo]]
*/
- def getApplicationInfoByTag(tag: String): Map[String, String]
+ def getApplicationInfoByTag(tag: String): ApplicationInfo
}
-object ApplicationOperation {
+object ApplicationState extends Enumeration {
+ type ApplicationState = Value
+ val PENDING, RUNNING, FINISHED, KILLED, FAILED, NOT_FOUND = Value
- /**
- * identifier determined by cluster manager for the engine
- */
- val APP_ID_KEY = "id"
- val APP_NAME_KEY = "name"
- val APP_STATE_KEY = "state"
- val APP_URL_KEY = "url"
- val APP_ERROR_KEY = "error"
+ def fromName(name: String): ApplicationState = name.toUpperCase match {
+ // k8s has some different status string
+ case "WAITING" => PENDING
Review Comment:
where is submitted/new/accepted/failed etc
--
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 #3137: [KYUUBI #3136] Make application info stable
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#issuecomment-1193782549
cc @yaooqinn @turboFei @zwangsheng
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#issuecomment-1193811046
> > it seems hard to maintain across different cluster managers
>
> yeah, a bit hard. I think we should handle this otherwise the end user will go to harder ..
I don't quite get why a map is harder?
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932832135
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/KubernetesApplicationOperation.scala:
##########
@@ -136,4 +136,15 @@ class KubernetesApplicationOperation extends ApplicationOperation with Logging {
object KubernetesApplicationOperation {
val LABEL_KYUUBI_UNIQUE_KEY = "kyuubi-unique-tag"
+
+ def toApplicationState(state: String): ApplicationState = state match {
+ // https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/types.go#L2396
+ // https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/
+ case "Pending" => PENDING
+ case "Running" => RUNNING
+ case "Succeeded" => FINISHED
+ case "Failed" | "Error" => FAILED
+ case "Unknown" => NOT_FOUND
+ case _ => throw new IllegalStateException("Not support state: " + state)
Review Comment:
added unknown state
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r930691407
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ApplicationOperation.scala:
##########
@@ -55,21 +56,44 @@ trait ApplicationOperation {
* Get the engine/application status by the unique application tag
*
* @param tag the unique application tag for engine instance.
- * @return a map contains the application status
+ * @return [[ApplicationInfo]]
*/
- def getApplicationInfoByTag(tag: String): Map[String, String]
+ def getApplicationInfoByTag(tag: String): ApplicationInfo
}
-object ApplicationOperation {
+object ApplicationState extends Enumeration {
+ type ApplicationState = Value
+ val PENDING, RUNNING, FINISHED, KILLED, FAILED, ZOMBIE, NOT_FOUND = Value
+}
- /**
- * identifier determined by cluster manager for the engine
- */
- val APP_ID_KEY = "id"
- val APP_NAME_KEY = "name"
- val APP_STATE_KEY = "state"
- val APP_URL_KEY = "url"
- val APP_ERROR_KEY = "error"
+case class ApplicationInfo(
+ id: String,
+ name: String,
+ state: ApplicationState,
+ url: Option[String] = None,
+ error: Option[String] = None) {
+ def toKeyValueList: Seq[Seq[String]] = {
Review Comment:
toMap?
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932065182
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/KubernetesApplicationOperation.scala:
##########
@@ -136,4 +136,15 @@ class KubernetesApplicationOperation extends ApplicationOperation with Logging {
object KubernetesApplicationOperation {
val LABEL_KYUUBI_UNIQUE_KEY = "kyuubi-unique-tag"
+
+ def applicationStateMapping(state: String): ApplicationState = state match {
Review Comment:
toApplicationState
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#issuecomment-1197551529
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137?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 [#3137](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e0a02e4) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/91a2534903b6eb0f9fe96a03b9c46c5085e401bc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91a2534) will **decrease** coverage by `0.00%`.
> The diff coverage is `56.00%`.
> :exclamation: Current head e0a02e4 differs from pull request most recent head 1f9047e. Consider uploading reports for the commit 1f9047e to get more accurate results
```diff
@@ Coverage Diff @@
## master #3137 +/- ##
============================================
- Coverage 51.38% 51.38% -0.01%
Complexity 6 6
============================================
Files 456 456
Lines 25328 25360 +32
Branches 3520 3538 +18
============================================
+ Hits 13016 13030 +14
- Misses 11064 11072 +8
- Partials 1248 1258 +10
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...kyuubi/engine/KubernetesApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvS3ViZXJuZXRlc0FwcGxpY2F0aW9uT3BlcmF0aW9uLnNjYWxh) | `14.28% <0.00%> (-1.79%)` | :arrow_down: |
| [...pache/kyuubi/engine/KyuubiApplicationManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvS3l1dWJpQXBwbGljYXRpb25NYW5hZ2VyLnNjYWxh) | `82.35% <0.00%> (ø)` | |
| [.../apache/kyuubi/server/api/v1/BatchesResource.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL3YxL0JhdGNoZXNSZXNvdXJjZS5zY2FsYQ==) | `70.05% <54.54%> (ø)` | |
| [...g/apache/kyuubi/operation/BatchJobSubmission.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137/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) | `74.40% <60.00%> (-2.62%)` | :arrow_down: |
| [...pache/kyuubi/engine/YarnApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvWWFybkFwcGxpY2F0aW9uT3BlcmF0aW9uLnNjYWxh) | `67.92% <61.90%> (-10.13%)` | :arrow_down: |
| [...rg/apache/kyuubi/engine/ApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvQXBwbGljYXRpb25PcGVyYXRpb24uc2NhbGE=) | `94.11% <93.33%> (-5.89%)` | :arrow_down: |
| [...apache/kyuubi/engine/JpsApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvSnBzQXBwbGljYXRpb25PcGVyYXRpb24uc2NhbGE=) | `77.41% <100.00%> (-2.00%)` | :arrow_down: |
| [...che/kyuubi/ctl/cmd/delete/DeleteBatchCommand.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137/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-a3l1dWJpLWN0bC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jdGwvY21kL2RlbGV0ZS9EZWxldGVCYXRjaENvbW1hbmQuc2NhbGE=) | `50.00% <0.00%> (ø)` | |
| [...n/scala/org/apache/kyuubi/ctl/util/BatchUtil.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137/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-a3l1dWJpLWN0bC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jdGwvdXRpbC9CYXRjaFV0aWwuc2NhbGE=) | | |
| ... and [5 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/3137/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) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?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] ulysses-you commented on pull request #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#issuecomment-1197935543
@yaooqinn @turboFei any concern for 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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#issuecomment-1197962497
the mapping looks good to me.
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#issuecomment-1193787941
it seems hard to maintain across different cluster managers
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932840617
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/KubernetesApplicationOperation.scala:
##########
@@ -134,6 +134,19 @@ class KubernetesApplicationOperation extends ApplicationOperation with Logging {
}
}
-object KubernetesApplicationOperation {
+object KubernetesApplicationOperation extends Logging {
val LABEL_KYUUBI_UNIQUE_KEY = "kyuubi-unique-tag"
+
+ def toApplicationState(state: String): ApplicationState = state match {
+ // https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/types.go#L2396
+ // https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/
+ case "Pending" => PENDING
+ case "Running" => RUNNING
+ case "Succeeded" => FINISHED
+ case "Failed" | "Error" => FAILED
+ case "Unknown" => ApplicationState.UNKNOWN
+ case _ =>
+ warn(s"Not support state: $state, using UNKNOWN")
Review Comment:
updated
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r930691798
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/KubernetesApplicationOperation.scala:
##########
@@ -136,4 +136,16 @@ class KubernetesApplicationOperation extends ApplicationOperation with Logging {
object KubernetesApplicationOperation {
val LABEL_KYUUBI_UNIQUE_KEY = "kyuubi-unique-tag"
+
+ def applicationStateMapping(state: String): ApplicationState = state match {
+ // https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/types.go#L2396
+ // https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/
+ case "Pending" => PENDING
+ case "Running" => RUNNING
+ case "Succeeded" => FINISHED
+ case "Failed" => FAILED
Review Comment:
| "Error"
--
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 #3137: [KYUUBI #3136] Make application info stable
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r928638171
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ApplicationOperation.scala:
##########
@@ -55,21 +56,55 @@ trait ApplicationOperation {
* Get the engine/application status by the unique application tag
*
* @param tag the unique application tag for engine instance.
- * @return a map contains the application status
+ * @return [[ApplicationInfo]]
*/
- def getApplicationInfoByTag(tag: String): Map[String, String]
+ def getApplicationInfoByTag(tag: String): ApplicationInfo
}
-object ApplicationOperation {
+object ApplicationState extends Enumeration {
+ type ApplicationState = Value
+ val PENDING, RUNNING, FINISHED, KILLED, FAILED, NOT_FOUND = Value
- /**
- * identifier determined by cluster manager for the engine
- */
- val APP_ID_KEY = "id"
- val APP_NAME_KEY = "name"
- val APP_STATE_KEY = "state"
- val APP_URL_KEY = "url"
- val APP_ERROR_KEY = "error"
+ def fromName(name: String): ApplicationState = name.toUpperCase match {
+ // k8s has some different status string
+ case "WAITING" => PENDING
+ case "DELETED" => KILLED
+ case "TERMINATED" => FINISHED
+ case "TERMINATING" => FINISHED
+ case "COMPLETED" => FINISHED
+ case "ERROR" => FAILED
+ case other => withName(other)
Review Comment:
this is a small bug fix that make kubernetes application status same with other
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932064049
##########
integration-tests/kyuubi-kubernetes-it/src/test/scala/org/apache/kyuubi/kubernetes/test/spark/SparkOnKubernetesTestsSuite.scala:
##########
@@ -143,20 +144,19 @@ class KyuubiOperationKubernetesClusterClientModeSuite
eventually(timeout(3.minutes), interval(50.milliseconds)) {
val state = k8sOperation.getApplicationInfoByTag(sessionHandle.identifier.toString)
- assert(state.nonEmpty)
- assert(state.contains("id"))
- assert(state.contains("name"))
- assert(state("state") === "RUNNING")
+ assert(state.id != null)
+ assert(state.name != null)
+ assert(state.state == RUNNING)
}
val killResponse = k8sOperation.killApplicationByTag(sessionHandle.identifier.toString)
assert(killResponse._1)
assert(killResponse._2 startsWith "Succeeded to terminate:")
val appInfo = k8sOperation.getApplicationInfoByTag(sessionHandle.identifier.toString)
- assert(!appInfo.contains("id"))
- assert(!appInfo.contains("name"))
- assert(appInfo("state") === "FINISHED")
+ assert(appInfo.id == null)
Review Comment:
new AppInfo(null, null, NOT_FOUND)
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r930722604
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -337,13 +335,21 @@ class BatchJobSubmission(
}
object BatchJobSubmission {
- def applicationFailed(applicationStatus: Option[Map[String, String]]): Boolean = {
- applicationStatus.map(_.get(ApplicationOperation.APP_STATE_KEY)).exists(s =>
- s.contains("KILLED") || s.contains("FAILED"))
+ def applicationFailed(applicationStatus: Option[ApplicationInfo]): Boolean = {
+ applicationStatus.map(_.state).exists {
+ case ApplicationState.FAILED => true
+ case ApplicationState.KILLED => true
+ case _ => false
+ }
}
- def applicationTerminated(applicationStatus: Option[Map[String, String]]): Boolean = {
- applicationStatus.map(_.get(ApplicationOperation.APP_STATE_KEY)).exists(s =>
- s.contains("KILLED") || s.contains("FAILED") || s.contains("FINISHED"))
+ def applicationTerminated(applicationStatus: Option[ApplicationInfo]): Boolean = {
+ applicationStatus.map(_.state).exists {
+ case ApplicationState.FAILED => true
+ case ApplicationState.KILLED => true
+ case ApplicationState.FINISHED => true
+ case ApplicationState.NOT_FOUND => true
Review Comment:
It should not happen. The caller side will check both process is alive and the application status, so if it's appliction status is not found that means the process also should not alived.
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r928676609
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ApplicationOperation.scala:
##########
@@ -55,21 +56,55 @@ trait ApplicationOperation {
* Get the engine/application status by the unique application tag
*
* @param tag the unique application tag for engine instance.
- * @return a map contains the application status
+ * @return [[ApplicationInfo]]
*/
- def getApplicationInfoByTag(tag: String): Map[String, String]
+ def getApplicationInfoByTag(tag: String): ApplicationInfo
}
-object ApplicationOperation {
+object ApplicationState extends Enumeration {
+ type ApplicationState = Value
+ val PENDING, RUNNING, FINISHED, KILLED, FAILED, NOT_FOUND = Value
- /**
- * identifier determined by cluster manager for the engine
- */
- val APP_ID_KEY = "id"
- val APP_NAME_KEY = "name"
- val APP_STATE_KEY = "state"
- val APP_URL_KEY = "url"
- val APP_ERROR_KEY = "error"
+ def fromName(name: String): ApplicationState = name.toUpperCase match {
+ // k8s has some different status string
+ case "WAITING" => PENDING
Review Comment:
will upadate it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] ulysses-you commented on a diff in pull request #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r930704607
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -337,13 +335,21 @@ class BatchJobSubmission(
}
object BatchJobSubmission {
- def applicationFailed(applicationStatus: Option[Map[String, String]]): Boolean = {
- applicationStatus.map(_.get(ApplicationOperation.APP_STATE_KEY)).exists(s =>
- s.contains("KILLED") || s.contains("FAILED"))
+ def applicationFailed(applicationStatus: Option[ApplicationInfo]): Boolean = {
+ applicationStatus.map(_.state).exists {
+ case ApplicationState.FAILED => true
+ case ApplicationState.KILLED => true
+ case _ => false
+ }
}
- def applicationTerminated(applicationStatus: Option[Map[String, String]]): Boolean = {
- applicationStatus.map(_.get(ApplicationOperation.APP_STATE_KEY)).exists(s =>
- s.contains("KILLED") || s.contains("FAILED") || s.contains("FINISHED"))
+ def applicationTerminated(applicationStatus: Option[ApplicationInfo]): Boolean = {
+ applicationStatus.map(_.state).exists {
+ case ApplicationState.FAILED => true
+ case ApplicationState.KILLED => true
+ case ApplicationState.FINISHED => true
+ case ApplicationState.NOT_FOUND => true
Review Comment:
yes, I thought it. For now the NOT_FOUND is only used for jps so it more likely terminated.
--
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 closed pull request #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you closed pull request #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
URL: https://github.com/apache/incubator-kyuubi/pull/3137
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r930707160
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -337,13 +335,21 @@ class BatchJobSubmission(
}
object BatchJobSubmission {
- def applicationFailed(applicationStatus: Option[Map[String, String]]): Boolean = {
- applicationStatus.map(_.get(ApplicationOperation.APP_STATE_KEY)).exists(s =>
- s.contains("KILLED") || s.contains("FAILED"))
+ def applicationFailed(applicationStatus: Option[ApplicationInfo]): Boolean = {
+ applicationStatus.map(_.state).exists {
+ case ApplicationState.FAILED => true
+ case ApplicationState.KILLED => true
+ case _ => false
+ }
}
- def applicationTerminated(applicationStatus: Option[Map[String, String]]): Boolean = {
- applicationStatus.map(_.get(ApplicationOperation.APP_STATE_KEY)).exists(s =>
- s.contains("KILLED") || s.contains("FAILED") || s.contains("FINISHED"))
+ def applicationTerminated(applicationStatus: Option[ApplicationInfo]): Boolean = {
+ applicationStatus.map(_.state).exists {
+ case ApplicationState.FAILED => true
+ case ApplicationState.KILLED => true
+ case ApplicationState.FINISHED => true
+ case ApplicationState.NOT_FOUND => true
Review Comment:
it can be not started yet
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932066445
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/BatchesResourceSuite.scala:
##########
@@ -437,10 +437,12 @@ class BatchesResourceSuite extends KyuubiFunSuite with RestFrontendTestHelper wi
assert(session2.createTime === batchMetadata2.createTime)
eventually(timeout(5.seconds)) {
- assert(session1.batchJobSubmissionOp.getStatus.state === OperationState.RUNNING)
+ assert(session1.batchJobSubmissionOp.getStatus.state === OperationState.RUNNING ||
+ session1.batchJobSubmissionOp.getStatus.state === OperationState.FINISHED)
assert(session1.batchJobSubmissionOp.builder.processLaunched)
- assert(session2.batchJobSubmissionOp.getStatus.state === OperationState.RUNNING)
+ assert(session2.batchJobSubmissionOp.getStatus.state === OperationState.RUNNING ||
+ session2.batchJobSubmissionOp.getStatus.state === OperationState.FINISHED)
Review Comment:
why these happens in a refactoring
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#issuecomment-1193825119
> I don't quite get why a map is harder? can you show us the use cases and reasons?
The caller side always use map.get(id/name/state/url/error) every time due to the user-face structure of application info is a certain class, e.g. `Batch` and also in kyuubi-ctl. That means these filed are soft required rather than optional.
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#issuecomment-1198940301
thank you for review, 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] ulysses-you commented on pull request #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#issuecomment-1193803516
> it seems hard to maintain across different cluster managers
yeah, a bit hard. I think we should handle this otherwise the end user will go to harder ..
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r928693397
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/ApplicationOperation.scala:
##########
@@ -55,21 +56,55 @@ trait ApplicationOperation {
* Get the engine/application status by the unique application tag
*
* @param tag the unique application tag for engine instance.
- * @return a map contains the application status
+ * @return [[ApplicationInfo]]
*/
- def getApplicationInfoByTag(tag: String): Map[String, String]
+ def getApplicationInfoByTag(tag: String): ApplicationInfo
}
-object ApplicationOperation {
+object ApplicationState extends Enumeration {
+ type ApplicationState = Value
+ val PENDING, RUNNING, FINISHED, KILLED, FAILED, NOT_FOUND = Value
- /**
- * identifier determined by cluster manager for the engine
- */
- val APP_ID_KEY = "id"
- val APP_NAME_KEY = "name"
- val APP_STATE_KEY = "state"
- val APP_URL_KEY = "url"
- val APP_ERROR_KEY = "error"
+ def fromName(name: String): ApplicationState = name.toUpperCase match {
+ // k8s has some different status string
+ case "WAITING" => PENDING
Review Comment:
any unhandled states from kubernetes, yunicorn, or valcano etc?
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932814430
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/KubernetesApplicationOperation.scala:
##########
@@ -136,4 +136,15 @@ class KubernetesApplicationOperation extends ApplicationOperation with Logging {
object KubernetesApplicationOperation {
val LABEL_KYUUBI_UNIQUE_KEY = "kyuubi-unique-tag"
+
+ def toApplicationState(state: String): ApplicationState = state match {
+ // https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/types.go#L2396
+ // https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/
+ case "Pending" => PENDING
+ case "Running" => RUNNING
+ case "Succeeded" => FINISHED
+ case "Failed" | "Error" => FAILED
+ case "Unknown" => NOT_FOUND
+ case _ => throw new IllegalStateException("Not support state: " + state)
Review Comment:
can we map it to unknown?
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/YarnApplicationOperation.scala:
##########
@@ -105,3 +108,17 @@ class YarnApplicationOperation extends ApplicationOperation with Logging {
}
}
}
+
+object YarnApplicationOperation {
+ def toApplicationState(state: YarnApplicationState): ApplicationState = state match {
+ case YarnApplicationState.NEW => ApplicationState.PENDING
+ case YarnApplicationState.NEW_SAVING => ApplicationState.PENDING
+ case YarnApplicationState.SUBMITTED => ApplicationState.PENDING
+ case YarnApplicationState.ACCEPTED => ApplicationState.PENDING
+ case YarnApplicationState.RUNNING => ApplicationState.RUNNING
+ case YarnApplicationState.FINISHED => ApplicationState.FINISHED
+ case YarnApplicationState.FAILED => ApplicationState.FAILED
+ case YarnApplicationState.KILLED => ApplicationState.KILLED
+ case _ => throw new IllegalStateException("Not support state: " + state)
Review Comment:
can we map it to unknown?
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932815459
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -337,13 +335,21 @@ class BatchJobSubmission(
}
object BatchJobSubmission {
- def applicationFailed(applicationStatus: Option[Map[String, String]]): Boolean = {
- applicationStatus.map(_.get(ApplicationOperation.APP_STATE_KEY)).exists(s =>
- s.contains("KILLED") || s.contains("FAILED"))
+ def applicationFailed(applicationStatus: Option[ApplicationInfo]): Boolean = {
+ applicationStatus.map(_.state).exists {
+ case ApplicationState.FAILED => true
+ case ApplicationState.KILLED => true
+ case _ => false
+ }
}
- def applicationTerminated(applicationStatus: Option[Map[String, String]]): Boolean = {
- applicationStatus.map(_.get(ApplicationOperation.APP_STATE_KEY)).exists(s =>
- s.contains("KILLED") || s.contains("FAILED") || s.contains("FINISHED"))
+ def applicationTerminated(applicationStatus: Option[ApplicationInfo]): Boolean = {
+ applicationStatus.map(_.state).exists {
+ case ApplicationState.FAILED => true
+ case ApplicationState.KILLED => true
+ case ApplicationState.FINISHED => true
+ case ApplicationState.NOT_FOUND => true
Review Comment:
comment it into code
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932835836
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/KubernetesApplicationOperation.scala:
##########
@@ -134,6 +134,19 @@ class KubernetesApplicationOperation extends ApplicationOperation with Logging {
}
}
-object KubernetesApplicationOperation {
+object KubernetesApplicationOperation extends Logging {
val LABEL_KYUUBI_UNIQUE_KEY = "kyuubi-unique-tag"
+
+ def toApplicationState(state: String): ApplicationState = state match {
+ // https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/types.go#L2396
+ // https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/
+ case "Pending" => PENDING
+ case "Running" => RUNNING
+ case "Succeeded" => FINISHED
+ case "Failed" | "Error" => FAILED
+ case "Unknown" => ApplicationState.UNKNOWN
+ case _ =>
+ warn(s"Not support state: $state, using UNKNOWN")
Review Comment:
Please make the warning message meaningful and illustrative if you are warning people.
> The kubernetes driver pod state: $state is not supported, mark the application state as UNKNOWN.
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/YarnApplicationOperation.scala:
##########
@@ -105,3 +108,19 @@ class YarnApplicationOperation extends ApplicationOperation with Logging {
}
}
}
+
+object YarnApplicationOperation extends Logging {
+ def toApplicationState(state: YarnApplicationState): ApplicationState = state match {
+ case YarnApplicationState.NEW => ApplicationState.PENDING
+ case YarnApplicationState.NEW_SAVING => ApplicationState.PENDING
+ case YarnApplicationState.SUBMITTED => ApplicationState.PENDING
+ case YarnApplicationState.ACCEPTED => ApplicationState.PENDING
+ case YarnApplicationState.RUNNING => ApplicationState.RUNNING
+ case YarnApplicationState.FINISHED => ApplicationState.FINISHED
+ case YarnApplicationState.FAILED => ApplicationState.FAILED
+ case YarnApplicationState.KILLED => ApplicationState.KILLED
+ case _ =>
+ warn(s"Not support state: $state, using UNKNOWN")
Review Comment:
ditto
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r930692955
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -337,13 +335,21 @@ class BatchJobSubmission(
}
object BatchJobSubmission {
- def applicationFailed(applicationStatus: Option[Map[String, String]]): Boolean = {
- applicationStatus.map(_.get(ApplicationOperation.APP_STATE_KEY)).exists(s =>
- s.contains("KILLED") || s.contains("FAILED"))
+ def applicationFailed(applicationStatus: Option[ApplicationInfo]): Boolean = {
+ applicationStatus.map(_.state).exists {
+ case ApplicationState.FAILED => true
+ case ApplicationState.KILLED => true
+ case _ => false
+ }
}
- def applicationTerminated(applicationStatus: Option[Map[String, String]]): Boolean = {
- applicationStatus.map(_.get(ApplicationOperation.APP_STATE_KEY)).exists(s =>
- s.contains("KILLED") || s.contains("FAILED") || s.contains("FINISHED"))
+ def applicationTerminated(applicationStatus: Option[ApplicationInfo]): Boolean = {
+ applicationStatus.map(_.state).exists {
+ case ApplicationState.FAILED => true
+ case ApplicationState.KILLED => true
+ case ApplicationState.FINISHED => true
+ case ApplicationState.NOT_FOUND => true
Review Comment:
NOT_FOUND is not always Terminated
--
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 #3137: [KYUUBI #3136] Make application info stable
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r928638785
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/engine/JpsApplicationOperation.scala:
##########
@@ -84,17 +84,14 @@ class JpsApplicationOperation extends ApplicationOperation {
killJpsApplicationByTag(tag, true)
}
- override def getApplicationInfoByTag(tag: String): Map[String, String] = {
+ override def getApplicationInfoByTag(tag: String): ApplicationInfo = {
val commandOption = getEngine(tag)
if (commandOption.nonEmpty) {
val idAndCmd = commandOption.get
val (id, cmd) = idAndCmd.splitAt(idAndCmd.indexOf(" "))
- Map(
- APP_ID_KEY -> id,
- APP_NAME_KEY -> cmd,
- APP_STATE_KEY -> "RUNNING")
+ ApplicationInfo(id = id, name = cmd, state = ApplicationState.RUNNING)
} else {
- Map(APP_STATE_KEY -> "FINISHED")
+ ApplicationInfo(id = null, name = null, state = ApplicationState.NOT_FOUND)
Review Comment:
it should be not found rather than finished for jps if process is not found ?
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932075207
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -74,7 +73,7 @@ class BatchJobSubmission(
private[kyuubi] val batchId: String = session.handle.identifier.toString
- private var applicationStatus: Option[Map[String, String]] = None
+ private var applicationStatus: Option[ApplicationInfo] = None
Review Comment:
applicationInfo
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -99,7 +98,7 @@ class BatchJobSubmission(
}
}
- private[kyuubi] def currentApplicationState: Option[Map[String, String]] = {
+ private[kyuubi] def currentApplicationState: Option[ApplicationInfo] = {
Review Comment:
currentApplicationInfo
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932167961
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/operation/BatchJobSubmission.scala:
##########
@@ -99,7 +98,7 @@ class BatchJobSubmission(
}
}
- private[kyuubi] def currentApplicationState: Option[Map[String, String]] = {
+ private[kyuubi] def currentApplicationState: Option[ApplicationInfo] = {
Review Comment:
addressed
--
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 #3137: [KYUUBI #3136] Change Map to a case class ApplicationInfo as the application info holder
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on code in PR #3137:
URL: https://github.com/apache/incubator-kyuubi/pull/3137#discussion_r932167325
##########
kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/BatchesResourceSuite.scala:
##########
@@ -437,10 +437,12 @@ class BatchesResourceSuite extends KyuubiFunSuite with RestFrontendTestHelper wi
assert(session2.createTime === batchMetadata2.createTime)
eventually(timeout(5.seconds)) {
- assert(session1.batchJobSubmissionOp.getStatus.state === OperationState.RUNNING)
+ assert(session1.batchJobSubmissionOp.getStatus.state === OperationState.RUNNING ||
+ session1.batchJobSubmissionOp.getStatus.state === OperationState.FINISHED)
assert(session1.batchJobSubmissionOp.builder.processLaunched)
- assert(session2.batchJobSubmissionOp.getStatus.state === OperationState.RUNNING)
+ assert(session2.batchJobSubmissionOp.getStatus.state === OperationState.RUNNING ||
+ session2.batchJobSubmissionOp.getStatus.state === OperationState.FINISHED)
Review Comment:
it's a flaky test before this pr, see https://github.com/apache/incubator-kyuubi/pull/3159
--
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