You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/04/15 03:38:48 UTC
[GitHub] [spark] AngersZhuuuu opened a new pull request, #36207: [SPARK-38910][YARN] In cluster mode, need to clean staging dir when spark.yarn.submit.waitAppCompletion=false and job failed after submitApplication quickly
AngersZhuuuu opened a new pull request, #36207:
URL: https://github.com/apache/spark/pull/36207
### What changes were proposed in this pull request?
In yarn `Client.run()`
```
def run(): Unit = {
submitApplication()
if (!launcherBackend.isConnected() && fireAndForget) {
val report = getApplicationReport(appId)
val state = report.getYarnApplicationState
logInfo(s"Application report for $appId (state: $state)")
logInfo(formatReportDetails(report, getDriverLogsLink(report)))
if (state == YarnApplicationState.FAILED || state == YarnApplicationState.KILLED) {
throw new SparkException(s"Application $appId finished with status: $state")
}
} else {
val YarnAppReport(appState, finalState, diags) = monitorApplication(appId)
if (appState == YarnApplicationState.FAILED || finalState == FinalApplicationStatus.FAILED) {
var amContainerSucceed = false
val amContainerExitMsg = s"AM Container for " +
s"${yarnClient.getApplicationReport(appId).getCurrentApplicationAttemptId} " +
s"exited with exitCode: 0"
diags.foreach { err =>
logError(s"Application diagnostics message: $err")
if (err.contains(amContainerExitMsg)) {
amContainerSucceed = true
```
Not clean the staging dir when match case
```
!launcherBackend.isConnected() && fireAndForget
```
This pr add clean staging dir for this case
### Why are the changes needed?
Clean staging dir
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
--
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
[GitHub] [spark] tgravescs commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
tgravescs commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1177651257
what exception was thrown here that caused the problem? Description should state.
This was likely in this order because if you don't unregister with YARN it could retry the application, if the staging directory is gone, that retry will now fail since application jar not there. Since the unregistered failed its possible YARN doesn't know it succeeded so then it could retry and all you could see if missing staging directory and you have to go searching for the real error.
I don't see reproduce case or a test here so it would be nice to test this to see what actually 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: 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
[GitHub] [spark] tgravescs commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
tgravescs commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1177748279
Note you have to take into account all running modes: cluster, client managed and unmanaged. If you are just thinking of client here, what about cluster mode. If the application is in cluster mode and it fails to unregistered and the Application master process dies, I would expect YARN to assume something bad happened and to rerun it. If you remove the staging directory that rerun is going to fail. That may not be ideal but it is better than the job showing up as failed to the user.
--
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
[GitHub] [spark] AngersZhuuuu commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1177772928
> Note you have to take into account all running modes: cluster, client managed and unmanaged. If you are just thinking of client here, what about cluster mode. If the application is in cluster mode and it fails to unregistered and the Application master process dies, I would expect YARN to assume something bad happened and to rerun it. If you remove the staging directory that rerun is going to fail. That may not be ideal but it is better than the job showing up as failed to the user.
In both cluster or client mode, rerun will upload the staging dir again, event the staging dir exists
<img width="1084" alt="截屏2022-07-07 下午11 13 24" src="https://user-images.githubusercontent.com/46485123/177808821-57a3ba3e-1876-456d-ad3a-c9376e29c0cf.png">
```
public static boolean copy(FileSystem srcFS, Path src, FileSystem dstFS, Path dst, boolean deleteSource, Configuration conf) throws IOException {
return copy(srcFS, src, dstFS, dst, deleteSource, true, conf);
}
```
so I don't think it will cause any problem to delete the staging dir here...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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
[GitHub] [spark] AngersZhuuuu commented on a diff in pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #36207:
URL: https://github.com/apache/spark/pull/36207#discussion_r851080385
##########
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?
Make a mistake, have update the pr desc.
--
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
[GitHub] [spark] tgravescs commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
tgravescs commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1177732373
That doesn't really answer my question. I get that it won't clean the staging directory, but I'm less concerned with that then it not retrying when it should. In your case, what is the yarn final application status when the unregister fails? the unregister is what gives YARN the final status of the application, without that is YARN assuming its successful if the application master goes away?
For unmanaged AM case the final status may not be success, so what happens there?
Note, I also have issues with the description here, in the jira you should describe the problem, not point to where you think there is a problem. So if you see an exception state that, put the exception in there, description on reproducing, etc. Then we can look for solution or what causes 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: 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
[GitHub] [spark] AngersZhuuuu commented on a diff in pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #36207:
URL: https://github.com/apache/spark/pull/36207#discussion_r851842745
##########
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:
> Yeah either way there's a possible issue - what if cleaning the staging dir fails now? It might be overkill, but, we could make sure that if cleanup fails, it still proceeds. try-catch here or in the clean method?
In clean method, there is already an ry-catch. Since I have seen some `unregister` failed, so here change the order is necessary.
--
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
[GitHub] [spark] tgravescs commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
tgravescs commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1179080164
Please put up pr to revert this. I’m out for a week or so so might not be able to review. @srowen @AngersZhuuuu
--
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
[GitHub] [spark] srowen commented on a diff in pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #36207:
URL: https://github.com/apache/spark/pull/36207#discussion_r851249509
##########
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:
Yeah either way there's a possible issue - what if cleaning the staging dir fails now?
It might be overkill, but, we could make sure that if cleanup fails, it still proceeds. try-catch here or in the clean method?
--
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
[GitHub] [spark] srowen commented on a diff in pull request #36207: [SPARK-38910][YARN] In cluster mode, need to clean staging dir when spark.yarn.submit.waitAppCompletion=false and job failed after submitApplication quickly
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #36207:
URL: https://github.com/apache/spark/pull/36207#discussion_r851046128
##########
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?
--
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
[GitHub] [spark] srowen commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1100879025
Try retriggering tests (or post a link to the successful action in your account). I think the PR builder is still not working.
(consider a try-catch per comments above too)
--
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
[GitHub] [spark] AngersZhuuuu commented on a diff in pull request #36207: [SPARK-38910][YARN] In cluster mode, need to clean staging dir when spark.yarn.submit.waitAppCompletion=false and job failed after submitApplication quickly
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #36207:
URL: https://github.com/apache/spark/pull/36207#discussion_r851080385
##########
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?
Remove this code, it's not correct.
--
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
[GitHub] [spark] srowen closed pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
URL: https://github.com/apache/spark/pull/36207
--
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
[GitHub] [spark] AngersZhuuuu commented on pull request #36207: [SPARK-38910][YARN] In cluster mode, need to clean staging dir when spark.yarn.submit.waitAppCompletion=false and job failed after submitApplication quickly
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1100000944
Application master will clean this dir for cluster mode.
--
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
[GitHub] [spark] AngersZhuuuu commented on a diff in pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by 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
[GitHub] [spark] AngersZhuuuu commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1101062777
Here https://github.com/AngersZhuuuu/spark/actions/runs/2181914576 can find the latest test. Strange...
--
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
[GitHub] [spark] tgravescs commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
tgravescs commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1189358719
thanks @dongjoon-hyun, just for reference, commit with revert: https://github.com/apache/spark/commit/9ebd7fb7317b32f57cffda8610c5d6fd4308302e
--
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
[GitHub] [spark] AngersZhuuuu commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1181258817
@tgravescs After re-check the whole logic, I got your point, although here said we don't need to rerun, but if it reregister failed, and it's not the last attempt, yarn will rerun this job.
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1181983884
This commit is reverted according to the above discussion.
--
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
[GitHub] [spark] tgravescs commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
tgravescs commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1177654286
Also note you changed this in one location but there are other locations for unmanaged AM's that still do it in this order, so we should be consistent.
--
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
[GitHub] [spark] tgravescs commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
tgravescs commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1177833836
how can cluster mode re-upload the staging directory when the client process doesn't have to exist anymore? You can launch a spark application and the client can go away. At that point the files are only in the staging directory so if you remove them they are gone. to be clear, I'm not talking about user rerunning the application, I'm talking about YARN retry due to unexpected failures.
--
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
[GitHub] [spark] AngersZhuuuu commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1177673970
> what exception was thrown here that caused the problem? Description should state. @AngersZhuuuu
>
> This was likely in this order because if you don't unregister with YARN it could retry the application, if the staging directory is gone, that retry will now fail since application jar not there. Since the unregistered failed its possible YARN doesn't know it succeeded so then it could retry and all you could see if missing staging directory and you have to go searching for the real error.
>
> I don't see reproduce case or a test here so it would be nice to test this to see what actually happens.
The problem is not when the code is running to here, the application success. but unregister timeout caused by yarn's bad performance, then it won't clean the staging dir. It won't impact rerun logic since here we should not rerun.
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1181981183
@AngersZhuuuu . According to the follow-up PR's content, I believe we had better revert this commit and start as non-followup PR freshly.
--
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
[GitHub] [spark] AngersZhuuuu commented on pull request #36207: [SPARK-38910][YARN] In cluster mode, need to clean staging dir when spark.yarn.submit.waitAppCompletion=false and job failed after submitApplication quickly
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1099821476
ping @srowen
--
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
[GitHub] [spark] AngersZhuuuu commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1101062393
@srowen Seems still can't trigger
--
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
[GitHub] [spark] srowen commented on pull request #36207: [SPARK-38910][YARN] Clean spark staging before `unregister`
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #36207:
URL: https://github.com/apache/spark/pull/36207#issuecomment-1101537654
Merged to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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
[GitHub] [spark] AngersZhuuuu commented on a diff in pull request #36207: [SPARK-38910][YARN] In cluster mode, need to clean staging dir when spark.yarn.submit.waitAppCompletion=false and job failed after submitApplication quickly
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on code in PR #36207:
URL: https://github.com/apache/spark/pull/36207#discussion_r851080277
##########
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
[GitHub] [spark] AngersZhuuuu commented on a diff in pull request #36207: [SPARK-38910][YARN] In cluster mode, need to clean staging dir when spark.yarn.submit.waitAppCompletion=false and job failed after submitApplication quickly
Posted by 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
--
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