You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@livy.apache.org by GitBox <gi...@apache.org> on 2020/08/14 01:05:51 UTC

[GitHub] [incubator-livy] AFFogarty opened a new pull request #303: [WIP] Add App state to AppInfo

AFFogarty opened a new pull request #303:
URL: https://github.com/apache/incubator-livy/pull/303


   ## What changes were proposed in this pull request?
   
   This PR adds App state to `AppInfo`.  This change gives clients more power to understand the state of their application and debug failures.
   
   For example, there is currently no way to differentiate between interactive sessions that have gone into YARN state `FAILED` or `FINISHED`. Livy reports both of these as `dead`.  With this new change, a client could inspect the `appState` to see the true state of the YARN application.
   
   ## How was this patch tested?
   
   (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
   (If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
   
   Please review https://livy.incubator.apache.org/community/ before opening 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.

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

[GitHub] [incubator-livy] andrasbeni commented on pull request #303: Add app state to AppInfo

Posted by GitBox <gi...@apache.org>.
andrasbeni commented on pull request #303:
URL: https://github.com/apache/incubator-livy/pull/303#issuecomment-681752556


   @AFFogarty, I'm curious what you think of the following alternatives:
   - Mapping SparkApp.State.FAILED to SessionState.ERROR()
   - Introducing SessionState.FAILED()
   I'm not too familiar with the state transitions, so above ideas may be dumb. In which case I'm happy to learn why.
   


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

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

[GitHub] [incubator-livy] andrasbeni edited a comment on pull request #303: Add app state to AppInfo

Posted by GitBox <gi...@apache.org>.
andrasbeni edited a comment on pull request #303:
URL: https://github.com/apache/incubator-livy/pull/303#issuecomment-681752556


   @AFFogarty, I'm curious what you think of the following alternatives:
   - Mapping SparkApp.State.FAILED to SessionState.ERROR()
   - Introducing SessionState.FAILED()
   
   I'm not too familiar with the state transitions, so above ideas may be dumb. In which case I'm happy to learn why.
   
   Other than that, I think you should create a jira issue and add its ID to the commit message
   


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

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

[GitHub] [incubator-livy] imback82 commented on pull request #303: Add app state to AppInfo

Posted by GitBox <gi...@apache.org>.
imback82 commented on pull request #303:
URL: https://github.com/apache/incubator-livy/pull/303#issuecomment-675683310


   @jerryshao Could you help reviewing this PR? Thanks in advance!


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

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

[GitHub] [incubator-livy] codecov-commenter commented on pull request #303: [WIP] Add app state to AppInfo

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #303:
URL: https://github.com/apache/incubator-livy/pull/303#issuecomment-674259807


   # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/303?src=pr&el=h1) Report
   > Merging [#303](https://codecov.io/gh/apache/incubator-livy/pull/303?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/97cf2f75929ef6c152afc468adbead269bd0758f&el=desc) will **increase** coverage by `0.02%`.
   > The diff coverage is `90.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-livy/pull/303/graphs/tree.svg?width=650&height=150&src=pr&token=0MkVbiUFwE)](https://codecov.io/gh/apache/incubator-livy/pull/303?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master     #303      +/-   ##
   ============================================
   + Coverage     68.48%   68.51%   +0.02%     
   - Complexity      840      841       +1     
   ============================================
     Files           103      103              
     Lines          5940     5948       +8     
     Branches        898      898              
   ============================================
   + Hits           4068     4075       +7     
   - Misses         1312     1314       +2     
   + Partials        560      559       -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/303?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...rc/main/scala/org/apache/livy/utils/SparkApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/303/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya0FwcC5zY2FsYQ==) | `80.64% <87.50%> (+4.64%)` | `1.00 <0.00> (ø)` | |
   | [...la/org/apache/livy/server/batch/BatchSession.scala](https://codecov.io/gh/apache/incubator-livy/pull/303/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvYmF0Y2gvQmF0Y2hTZXNzaW9uLnNjYWxh) | `86.86% <100.00%> (+0.13%)` | `14.00 <0.00> (ø)` | |
   | [...e/livy/server/interactive/InteractiveSession.scala](https://codecov.io/gh/apache/incubator-livy/pull/303/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvaW50ZXJhY3RpdmUvSW50ZXJhY3RpdmVTZXNzaW9uLnNjYWxh) | `69.85% <100.00%> (+0.08%)` | `51.00 <0.00> (ø)` | |
   | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/303/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `62.79% <0.00%> (-2.33%)` | `0.00% <0.00%> (ø%)` | |
   | [...ain/scala/org/apache/livy/utils/SparkYarnApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/303/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya1lhcm5BcHAuc2NhbGE=) | `73.75% <0.00%> (-1.25%)` | `40.00% <0.00%> (ø%)` | |
   | [.../scala/org/apache/livy/sessions/SessionState.scala](https://codecov.io/gh/apache/incubator-livy/pull/303/diff?src=pr&el=tree#diff-Y29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvc2Vzc2lvbnMvU2Vzc2lvblN0YXRlLnNjYWxh) | `59.45% <0.00%> (ø)` | `0.00% <0.00%> (ø%)` | |
   | [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/303/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `80.83% <0.00%> (+0.83%)` | `45.00% <0.00%> (ø%)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/303?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/303?src=pr&el=footer). Last update [97cf2f7...4edc259](https://codecov.io/gh/apache/incubator-livy/pull/303?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

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

[GitHub] [incubator-livy] rapoth commented on pull request #303: Add app state to AppInfo

Posted by GitBox <gi...@apache.org>.
rapoth commented on pull request #303:
URL: https://github.com/apache/incubator-livy/pull/303#issuecomment-683925812


   @jerryshao Any chance you could review this PR? Thanks a lot for your time in advance!


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

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

[GitHub] [incubator-livy] imback82 commented on a change in pull request #303: Add app state to AppInfo

Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #303:
URL: https://github.com/apache/incubator-livy/pull/303#discussion_r471853029



##########
File path: server/src/test/scala/org/apache/livy/server/interactive/InteractiveSessionServletSpec.scala
##########
@@ -168,7 +168,10 @@ class InteractiveSessionServletSpec extends BaseInteractiveServletSpec {
     val proxyUser = "proxyUser"
     val state = SessionState.Running
     val kind = Spark
-    val appInfo = AppInfo(Some("DRIVER LOG URL"), Some("SPARK UI URL"))
+    val appInfo = AppInfo(
+      Some("DRIVER LOG URL"),
+      Some("SPARK UI URL"),
+      Some(SparkApp.State.RUNNING))

Review comment:
       can we also test `None` case?

##########
File path: server/src/main/scala/org/apache/livy/utils/SparkApp.scala
##########
@@ -19,17 +19,28 @@ package org.apache.livy.utils
 
 import scala.collection.JavaConverters._
 
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.module.scala.JsonScalaEnumeration
+
 import org.apache.livy.LivyConf
+import org.apache.livy.utils.SparkApp.StateTypeReference
 
 object AppInfo {
   val DRIVER_LOG_URL_NAME = "driverLogUrl"
   val SPARK_UI_URL_NAME = "sparkUiUrl"
+  val APP_STATE_NAME = "appState"
 }
 
-case class AppInfo(var driverLogUrl: Option[String] = None, var sparkUiUrl: Option[String] = None) {
+case class AppInfo(
+    var driverLogUrl: Option[String] = None,
+    var sparkUiUrl: Option[String] = None,
+    @JsonScalaEnumeration(classOf[StateTypeReference])
+    var appState: Option[SparkApp.State] = None) {
   import AppInfo._
   def asJavaMap: java.util.Map[String, String] =
-    Map(DRIVER_LOG_URL_NAME -> driverLogUrl.orNull, SPARK_UI_URL_NAME -> sparkUiUrl.orNull).asJava
+    Map(DRIVER_LOG_URL_NAME -> driverLogUrl.orNull,
+      SPARK_UI_URL_NAME -> sparkUiUrl.orNull,
+      APP_STATE_NAME -> appState.map(s => s.toString).orNull).asJava

Review comment:
       nit: `appState.map(_.toString)`




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

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

[GitHub] [incubator-livy] AFFogarty commented on pull request #303: Add app state to AppInfo

Posted by GitBox <gi...@apache.org>.
AFFogarty commented on pull request #303:
URL: https://github.com/apache/incubator-livy/pull/303#issuecomment-678436061


   Not sure why an unrelated test in `livy-rsc` failed this time.  Can we re-run the build?


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

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

[GitHub] [incubator-livy] AFFogarty commented on a change in pull request #303: Add app state to AppInfo

Posted by GitBox <gi...@apache.org>.
AFFogarty commented on a change in pull request #303:
URL: https://github.com/apache/incubator-livy/pull/303#discussion_r472444662



##########
File path: server/src/test/scala/org/apache/livy/server/interactive/InteractiveSessionServletSpec.scala
##########
@@ -168,7 +168,10 @@ class InteractiveSessionServletSpec extends BaseInteractiveServletSpec {
     val proxyUser = "proxyUser"
     val state = SessionState.Running
     val kind = Spark
-    val appInfo = AppInfo(Some("DRIVER LOG URL"), Some("SPARK UI URL"))
+    val appInfo = AppInfo(
+      Some("DRIVER LOG URL"),
+      Some("SPARK UI URL"),
+      Some(SparkApp.State.RUNNING))

Review comment:
       Added verification that `appState=None` gets serialized to `"appState": null`.




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

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

[GitHub] [incubator-livy] AFFogarty commented on a change in pull request #303: Add app state to AppInfo

Posted by GitBox <gi...@apache.org>.
AFFogarty commented on a change in pull request #303:
URL: https://github.com/apache/incubator-livy/pull/303#discussion_r472452018



##########
File path: server/src/main/scala/org/apache/livy/utils/SparkApp.scala
##########
@@ -19,17 +19,28 @@ package org.apache.livy.utils
 
 import scala.collection.JavaConverters._
 
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.module.scala.JsonScalaEnumeration
+
 import org.apache.livy.LivyConf
+import org.apache.livy.utils.SparkApp.StateTypeReference
 
 object AppInfo {
   val DRIVER_LOG_URL_NAME = "driverLogUrl"
   val SPARK_UI_URL_NAME = "sparkUiUrl"
+  val APP_STATE_NAME = "appState"
 }
 
-case class AppInfo(var driverLogUrl: Option[String] = None, var sparkUiUrl: Option[String] = None) {
+case class AppInfo(
+    var driverLogUrl: Option[String] = None,
+    var sparkUiUrl: Option[String] = None,
+    @JsonScalaEnumeration(classOf[StateTypeReference])
+    var appState: Option[SparkApp.State] = None) {
   import AppInfo._
   def asJavaMap: java.util.Map[String, String] =
-    Map(DRIVER_LOG_URL_NAME -> driverLogUrl.orNull, SPARK_UI_URL_NAME -> sparkUiUrl.orNull).asJava
+    Map(DRIVER_LOG_URL_NAME -> driverLogUrl.orNull,
+      SPARK_UI_URL_NAME -> sparkUiUrl.orNull,
+      APP_STATE_NAME -> appState.map(s => s.toString).orNull).asJava

Review comment:
       Fixed




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

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