Skip to content

[SPARK-20875] Spark should print the log when the directory has been deleted#18102

Closed
liu-zhaokun wants to merge 6 commits intoapache:masterfrom
liu-zhaokun:master_log
Closed

[SPARK-20875] Spark should print the log when the directory has been deleted#18102
liu-zhaokun wants to merge 6 commits intoapache:masterfrom
liu-zhaokun:master_log

Conversation

@liu-zhaokun
Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SPARK-20875
When the "deleteRecursively" method is invoked,spark doesn't print any log if the path was deleted.For example,spark only print "Removing directory" when the worker began cleaning spark.work.dir,but didn't print any log about "the path has been delete".So, I can't judge whether the path was deleted form the worker's logfile,If there is any accidents about Linux.

@jerryshao
Copy link
Copy Markdown
Contributor

I would doubt it will be too verbose to print out the path.

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

@jerryshao
Thanks for your reply.Do you mean that I need to streamline the content?

@sameeragarwal
Copy link
Copy Markdown
Member

Agree -- logging at INFO level is an overkill. At the very least it should be at the TRACE level.

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

@sameeragarwal
Thanks for your reply. I will make changes based on your opinion.

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

@jerryshao
Hi, I think change the log level also can make the logfile more concise.would this change eliminate your concerns?

@jerryshao
Copy link
Copy Markdown
Contributor

Looks like it will only print out the path of directory, not file, is that on purpose?

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

@jerryshao
Yes, I only want to provided a method to judge whether the spark.work.dir is removing or has been cleaned.

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

@jerryshao
Hello,thanks for your attention,is there any suggestion?

Copy link
Copy Markdown
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but you invoke string interpolation but then don't use it. The string message needs to be fixed.

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

@srowen
Thanks for your reply. I made changes based on your opinion.Please review it soon.

for (child <- listFilesSafely(file)) {
try {
deleteRecursively(child)
logTrace(file.getAbsolutePath + " has been deleted")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, why not s"${file.getAbsolutePath} has been deleted"? it's actually important for a log trace statement

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

@srowen
Thanks for your patience. I modified again.Please review it.

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

I found there are some mistakes.So I modify the PR again.Please review it.

} finally {
if (!file.delete()) {
if (file.delete()) {
logTrace(s"${file.getAbsolutePath} has been deleted")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should have the double spaced indentation.

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

@HyukjinKwon
Thanks for your reply. I made changes based on your opinion.Please review it soon.

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 26, 2017

Test build #3759 has finished for PR 18102 at commit 1a5853f.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

@srowen
Hello,the test said there is some failure,is IT my problems?

@liu-zhaokun
Copy link
Copy Markdown
Contributor Author

@HyukjinKwon
Hello,the test said there is some failure,is it my problems?

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 27, 2017

Test build #3765 has finished for PR 18102 at commit 1a5853f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Copy Markdown
Member

srowen commented May 27, 2017

Merged to master

@asfgit asfgit closed this in 8faffc4 May 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants