[SPARK-21721][SQL] Clear FileSystem deleteOnExit cache when paths are successfully removed #18934

viirya · 2017-08-14T04:17:52Z

What changes were proposed in this pull request?

We put staging path to delete into the deleteOnExit cache of FileSystem in case of the path can't be successfully removed. But when we successfully remove the path, we don't remove it from the cache. We should do it to avoid continuing grow the cache size.

How was this patch tested?

Added a test.

SparkQA · 2017-08-14T05:41:44Z

Test build #80609 has finished for PR 18934 at commit 1f8444d.

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

viirya · 2017-08-14T06:25:43Z

There is a test which doesn't drop the table it created...

SparkQA · 2017-08-14T07:04:49Z

Test build #80614 has finished for PR 18934 at commit 976e288.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-08-14T07:04:50Z

Test build #80613 has finished for PR 18934 at commit 8912397.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-08-14T07:59:00Z

retest this please.

SparkQA · 2017-08-14T10:01:59Z

Test build #80621 has finished for PR 18934 at commit 976e288.

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

srowen · 2017-08-14T10:03:39Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveTable.scala

+      createdTempDir.foreach { path =>
+        path.getFileSystem(hadoopConf).delete(path, true)
+        // If we successfully delete the staging directory, remove it from FileSystem's cache.
+        val fs = path.getFileSystem(hadoopConf)


You call getFileSystem twice; you can reuse it.
I suppose this adds an additional check to see if the file exists, but this won't be called that often.

Oh. right. Thanks.

srowen · 2017-08-14T10:04:32Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala

-
-    orders.toDF.createOrReplaceTempView("orders1")
-    orderUpdates.toDF.createOrReplaceTempView("orderupdates1")
+    withTable("orders", "orderupdates") {


Is this intended to be part of this PR?

The newly added test has been bitten by an existing test which doesn't drop created table. Those old tests all don't drop created tables. I can move them to another minor PR if you think it's necessary.

Usually, each test case should be independent. Could you isolate your new test case instead? It would be better when we backport your PR later.

Yes. Please revert all the unrelated changes.

SparkQA · 2017-08-14T13:29:53Z

Test build #80627 has finished for PR 18934 at commit 13defbb.

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

dbolshak · 2017-08-14T16:54:25Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveTable.scala

-      createdTempDir.foreach { path => path.getFileSystem(hadoopConf).delete(path, true) }
+      createdTempDir.foreach { path =>
+        val fs = path.getFileSystem(hadoopConf)
+        fs.delete(path, true)


FileSystem.delete returns true if delete is successful else false. So it makes sense to re-write a bit

if (fs.delete(path, true)) { fs.cancelDeleteOnExit(path) }

Great. Thanks.

SparkQA · 2017-08-15T02:04:13Z

Test build #80649 has finished for PR 18934 at commit 2b0fd74.

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

dongjoon-hyun

+1, LGTM.

gatorsmile · 2017-08-15T05:28:17Z

LGTM

… successfully removed ## What changes were proposed in this pull request? We put staging path to delete into the deleteOnExit cache of `FileSystem` in case of the path can't be successfully removed. But when we successfully remove the path, we don't remove it from the cache. We should do it to avoid continuing grow the cache size. ## How was this patch tested? Added a test. Author: Liang-Chi Hsieh <viirya@gmail.com> Closes #18934 from viirya/SPARK-21721. (cherry picked from commit 4c3cf1c) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

gatorsmile · 2017-08-15T05:30:01Z

Thanks! Merging to master/2.2

gatorsmile · 2017-08-15T05:33:40Z

Tried to merge to Spark 2.1, but I hit a conflict. Thus, please let me know if anybody wants it in 2.1. Thanks!

yzheng616 · 2017-08-15T06:30:29Z

Please try to fix it in 2.1 too. We have a product running on this version Spark. Thanks a lot!

gatorsmile · 2017-08-15T06:50:30Z

cc @viirya

viirya · 2017-08-15T07:24:08Z

@yzheng616 @gatorsmile Ok. I will backport it to 2.1.

cloud-fan · 2017-08-20T09:53:38Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala

+      sql("CREATE TABLE test21721 (key INT, value STRING)")
+      val pathSizeToDeleteOnExit = setOfPath.size()
+
+      (0 to 10).foreach(_ => testData.write.mode(SaveMode.Append).insertInto("test1"))


please fix the typo, should be test21721 instead of test1

Will fix now.

… successfully removed ## What changes were proposed in this pull request? We put staging path to delete into the deleteOnExit cache of `FileSystem` in case of the path can't be successfully removed. But when we successfully remove the path, we don't remove it from the cache. We should do it to avoid continuing grow the cache size. ## How was this patch tested? Added a test. Author: Liang-Chi Hsieh <viirya@gmail.com> Closes apache#18934 from viirya/SPARK-21721. (cherry picked from commit 4c3cf1c) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

viirya force-pushed the SPARK-21721 branch 2 times, most recently from 8912397 to 976e288 Compare August 14, 2017 06:55

srowen reviewed Aug 14, 2017

View reviewed changes

Clear FileSystem deleteOnExit cache when paths are successfully removed.

13defbb

viirya force-pushed the SPARK-21721 branch from 976e288 to 13defbb Compare August 14, 2017 10:16

dbolshak reviewed Aug 14, 2017

View reviewed changes

Use fs.delete returned value, revert the change to old tests.

2b0fd74

dongjoon-hyun approved these changes Aug 15, 2017

View reviewed changes

asfgit closed this in 4c3cf1c Aug 15, 2017

viirya mentioned this pull request Aug 15, 2017

[SPARK-21721][SQL][Backport-2.1] Clear FileSystem deleteOnExit cache when paths are successfully removed #18947

Closed

cloud-fan reviewed Aug 20, 2017

View reviewed changes

viirya deleted the SPARK-21721 branch December 27, 2023 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21721][SQL] Clear FileSystem deleteOnExit cache when paths are successfully removed #18934

[SPARK-21721][SQL] Clear FileSystem deleteOnExit cache when paths are successfully removed #18934

viirya commented Aug 14, 2017

SparkQA commented Aug 14, 2017

viirya commented Aug 14, 2017

SparkQA commented Aug 14, 2017

SparkQA commented Aug 14, 2017

viirya commented Aug 14, 2017

SparkQA commented Aug 14, 2017

srowen Aug 14, 2017

viirya Aug 14, 2017

srowen Aug 14, 2017

viirya Aug 14, 2017 •

edited

Loading

dongjoon-hyun Aug 14, 2017

gatorsmile Aug 14, 2017

viirya Aug 14, 2017

SparkQA commented Aug 14, 2017

dbolshak Aug 14, 2017

viirya Aug 14, 2017

SparkQA commented Aug 15, 2017

dongjoon-hyun left a comment

gatorsmile commented Aug 15, 2017

gatorsmile commented Aug 15, 2017

gatorsmile commented Aug 15, 2017

yzheng616 commented Aug 15, 2017

gatorsmile commented Aug 15, 2017

viirya commented Aug 15, 2017

cloud-fan Aug 20, 2017

viirya Aug 20, 2017

[SPARK-21721][SQL] Clear FileSystem deleteOnExit cache when paths are successfully removed #18934

[SPARK-21721][SQL] Clear FileSystem deleteOnExit cache when paths are successfully removed #18934

Conversation

viirya commented Aug 14, 2017

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Aug 14, 2017

viirya commented Aug 14, 2017

SparkQA commented Aug 14, 2017

SparkQA commented Aug 14, 2017

viirya commented Aug 14, 2017

SparkQA commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 15, 2017

dongjoon-hyun left a comment

Choose a reason for hiding this comment

gatorsmile commented Aug 15, 2017

gatorsmile commented Aug 15, 2017

gatorsmile commented Aug 15, 2017

yzheng616 commented Aug 15, 2017

gatorsmile commented Aug 15, 2017

viirya commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Aug 14, 2017 •

edited

Loading