[GitHub] [spark] AngersZhuuuu commented on a diff in pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`

[GitBox <gi...@apache.org>]

AngersZhuuuu commented on code in PR #36207:
URL: https://github.com/apache/spark/pull/36207#discussion_r851078958


##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala:
##########
@@ -1331,7 +1331,11 @@ private[spark] class Client(
       logInfo(s"Application report for $appId (state: $state)")
       logInfo(formatReportDetails(report, getDriverLogsLink(report)))
       if (state == YarnApplicationState.FAILED || state == YarnApplicationState.KILLED) {
+        cleanupStagingDir()
         throw new SparkException(s"Application $appId finished with status: $state")
+      } else {
+        val fs = stagingDirPath.getFileSystem(hadoopConf)
+        fs.deleteOnExit(stagingDirPath)

Review Comment:
   > Where is this otherwise cleaned up - is it also using deleteOnExit?
   
   1. If submitApplication failed, will clean staging https://github.com/apache/spark/blob/cc7cb7a803d5de03c526480c8968bbb2c3e82484/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala#L231
   
   2. When monitoring application, if ApplicationNotFound, will clean https://github.com/apache/spark/blob/cc7cb7a803d5de03c526480c8968bbb2c3e82484/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala#L1168-L1171
   3. When monitoring application, if Application end, will clean https://github.com/apache/spark/blob/cc7cb7a803d5de03c526480c8968bbb2c3e82484/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala#L1214-L1218
   



##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala:
##########
@@ -1331,7 +1331,11 @@ private[spark] class Client(
       logInfo(s"Application report for $appId (state: $state)")
       logInfo(formatReportDetails(report, getDriverLogsLink(report)))
       if (state == YarnApplicationState.FAILED || state == YarnApplicationState.KILLED) {
+        cleanupStagingDir()
         throw new SparkException(s"Application $appId finished with status: $state")
+      } else {
+        val fs = stagingDirPath.getFileSystem(hadoopConf)
+        fs.deleteOnExit(stagingDirPath)

Review Comment:
   If we set `spark.yarn.submit.waitAppCompletion=false`, here `fireAndForget=true`
   https://github.com/apache/spark/blob/cc7cb7a803d5de03c526480c8968bbb2c3e82484/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala#L1326-L1336
   
   If `launcherBackend ` just lose connection after `submitApplication`, means application success or failed quickly.
   Here we won't clean staging dir in current code, In this pr, there are two case:
   1. application just failed or killed, I think we need clean the staging dir directly.
   2. For other status, since client won't wait application complete. The client just exit, we need to find other method to resolve this.



-- 
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: reviews-unsubscribe@spark.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org