[SPARK-22227][CORE] DiskBlockManager.getAllBlocks now tolerates temp files #19458

superbobry · 2017-10-09T16:56:40Z

What changes were proposed in this pull request?

Prior to this commit getAllBlocks implicitly assumed that the directories
managed by the DiskBlockManager contain only the files corresponding to
valid block IDs. In reality, this assumption was violated during shuffle,
which produces temporary files in the same directory as the resulting
blocks. As a result, calls to getAllBlocks during shuffle were unreliable.

The fix could be made more efficient, but this is probably good enough.

How was this patch tested?

DiskBlockManagerSuite

srowen · 2017-10-09T17:10:06Z

core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala

-    getAllFiles().map(f => BlockId(f.getName))
+    // SPARK-22227: the Try guides against temporary files written
+    // during shuffle which do not correspond to valid block IDs.
+    getAllFiles().flatMap(f => Try(BlockId(f.getName)).toOption)


This has the effect of swallowing a number of possible exceptions. I think at least you'd want to unpack this and log the error. But is there a more explicit way of excluding temp files? it seems like getAllFiles shouldn't return those either?

This has the effect of swallowing a number of possible exceptions.
It does. Right now the only possible exception coming from BlockId.apply is IllegalStateException , but I agree that using Try makes the code fragile. My intention was to make an easy to read proof of concept and then adjust the patch according to the feedback.

What do you think about adding a "safe" parsing method to BlockId? The metho would return an Option instead of throwing and would only be used in DiskBlockManager for now? BlockId.apply can then be expressed parse(s).getOrElse { throw ... }.

Alernatively, we can expose Block.isValid which would combine the individual regexes into one.

But is there a more explicit way of excluding temp files? it seems like getAllFiles shouldn't return those either?

I think this is tricky to do without leaking some abstractions, e.g. the ".UUID" naming scheme in Utils.tempFileWith and Temp*BlockId naming scheme.

jiangxb1987

LGTM, cc @cloud-fan

jerryshao · 2017-10-12T03:39:25Z

Instead of filtering out temp blocks, why not adding parsing rule for TempLocalBlockId and TempShuffleBlockId? That could also solve the problem. Since DiskBlockManager#getAllFiles doesn't filter out temp shuffle/local files, is it better to keep the same behavior for DiskBlockManager#getAllBlocks? Also it would better for upstream code to decide whether to filter out temp files/blocks.

superbobry · 2017-10-12T09:26:21Z

Instead of filtering out temp blocks, why not adding parsing rule for TempLocalBlockId and TempShuffleBlockId?

Because this fixes the issue for 2 out of 3 possible temp files. The unhandled case is produced by Utils.tempFileWith.

Since DiskBlockManager#getAllFiles doesn't filter out temp shuffle/local files, is it better to keep the same behavior for DiskBlockManager#getAllBlocks?

I agree that it makes sense to keep those in sync, therefore I prefer to introduce Block.isValid and use it in getAllFiles.

Also it would better for upstream code to decide whether to filter out temp files/blocks.

Possibly, but in any case getAllBlocks should not throw, since temp blocks are an implementation detail.

jerryshao · 2017-10-12T09:32:31Z

Yes, I agree in any case it should not throw an exception. But in this PR you filtered out temp shuffle/local blocks, do you think this block is valid or not, are they blocks?

So I'd like not filtering out those blocks, instead adding two parsing rules for those blocks. And for any other illegal files (cannot be parsed) catch and log the exception.

superbobry · 2017-10-12T16:42:25Z

I would say that temporary shuffle blocks are also blocks.

As for DiskBlockManager.getAllFiles, the documentation states that it returns all files in the directories managed by DiskBlockManager. Filtering anything would violate this contract, WDYT?

jerryshao · 2017-10-13T01:56:19Z

core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala

@@ -100,7 +100,16 @@ private[spark] class DiskBlockManager(conf: SparkConf, deleteFilesOnStop: Boolea

  /** List all the blocks currently stored on disk by the disk manager. */
  def getAllBlocks(): Seq[BlockId] = {
-    getAllFiles().map(f => BlockId(f.getName))
+    getAllFiles().flatMap { f =>
+      val blockId = BlockId.guess(f.getName)


It looks not so necessary to define a new method guess for the use here only. I think here we can still use apply and catch/log the exception. In another word, we can simply changes apply() and use it here, defining new guess method is not so necessary.

+1 for using try-catch here.

Will do. Should I log the exception even if the file has been produced by Utils.tempFileWith?

I think we don't need to log here.

Do you mean we don't need to log at all?

jiangxb1987 · 2017-10-19T12:28:34Z

ping @superbobry

cloud-fan · 2017-10-23T22:38:30Z

core/src/main/scala/org/apache/spark/storage/BlockId.scala

@@ -100,6 +100,8 @@ private[spark] case class TestBlockId(id: String) extends BlockId {
  override def name: String = "test_" + id
 }

+class UnrecognizedBlockId(name: String) extends Exception


nit: extend SparkException

cloud-fan · 2017-10-23T22:42:54Z

core/src/main/scala/org/apache/spark/storage/DiskBlockManager.scala

+        case _: UnrecognizedBlockId =>
+          // This does not handle a special-case of a temporary file
+          // created by [[SortShuffleWriter]].
+          log.warn(s"Encountered an unexpected file in a managed directory: $f")


My question here: is it really something we need to warn users? What should they do about it? Here we are getting block ids from file names, so it's legal to meet a temporary file at this place.

I am not convinced we should log either. I've added this line because it was suggested by @jerryshao.

cloud-fan · 2017-10-24T11:53:58Z

retest this please

SparkQA · 2017-10-24T14:30:58Z

Test build #83014 has finished for PR 19458 at commit febdd7e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class UnrecognizedBlockId(name: String)

…files Prior to this commit getAllBlocks implicitly assumed that the directories managed by the DiskBlockManager contain only the files corresponding to valid block IDs. In reality this assumption was violated during shuffle, which produces temporary files in the same directory as the resulting blocks. As a result, calls to getAllBlocks during shuffle were unreliable. The fix could be made more efficient, but this is probably good enough.

This commit also introduces a safe alternative to BlockId.apply which return option instead of throwing an exception.

cloud-fan · 2017-10-25T06:21:46Z

retest this please

jerryshao · 2017-10-25T06:34:08Z

There's a UT failure (https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83014/testReport/junit/org.apache.spark.storage/BlockIdSuite/test_bad_deserialization/). @superbobry please fix this failure.

SparkQA · 2017-10-25T07:05:02Z

Test build #83034 has finished for PR 19458 at commit ff9a6ae.

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

jerryshao · 2017-10-25T12:24:24Z

retest this please.

SparkQA · 2017-10-25T15:10:57Z

Test build #83043 has finished for PR 19458 at commit 2b61347.

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

cloud-fan · 2017-10-25T15:19:04Z

retest this please

SparkQA · 2017-10-25T18:53:15Z

Test build #83048 has finished for PR 19458 at commit 2b61347.

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

…files Prior to this commit getAllBlocks implicitly assumed that the directories managed by the DiskBlockManager contain only the files corresponding to valid block IDs. In reality, this assumption was violated during shuffle, which produces temporary files in the same directory as the resulting blocks. As a result, calls to getAllBlocks during shuffle were unreliable. The fix could be made more efficient, but this is probably good enough. `DiskBlockManagerSuite` Author: Sergei Lebedev <s.lebedev@criteo.com> Closes #19458 from superbobry/block-id-option. (cherry picked from commit b377ef1) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2017-10-25T21:20:09Z

thanks, merging to master/2.2

…files Prior to this commit getAllBlocks implicitly assumed that the directories managed by the DiskBlockManager contain only the files corresponding to valid block IDs. In reality, this assumption was violated during shuffle, which produces temporary files in the same directory as the resulting blocks. As a result, calls to getAllBlocks during shuffle were unreliable. The fix could be made more efficient, but this is probably good enough. `DiskBlockManagerSuite` Author: Sergei Lebedev <s.lebedev@criteo.com> Closes apache#19458 from superbobry/block-id-option. (cherry picked from commit b377ef1) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

srowen reviewed Oct 9, 2017

View reviewed changes

jiangxb1987 approved these changes Oct 11, 2017

View reviewed changes

jerryshao reviewed Oct 13, 2017

View reviewed changes

cloud-fan reviewed Oct 23, 2017

View reviewed changes

Sergei Lebedev added 6 commits October 24, 2017 19:25

Allowed to parse Temp*BlockId in BlockId.apply

152b734

This commit also introduces a safe alternative to BlockId.apply which return option instead of throwing an exception.

Removed BlockId.guess as suggested in the comments

0da650a

Addressed another round of reviews

87a4ca1

Fixed failing test in BlockIdSuite

b794484

Updated BlockIdSuite with parsing tests for temp blocks

ff9a6ae

superbobry force-pushed the block-id-option branch from cf9db5c to ff9a6ae Compare October 24, 2017 17:36

Fixed DiskBlockManagerSuite

2b61347

asfgit closed this in b377ef1 Oct 25, 2017

Willymontaz deleted the block-id-option branch April 2, 2019 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-22227][CORE] DiskBlockManager.getAllBlocks now tolerates temp files #19458

[SPARK-22227][CORE] DiskBlockManager.getAllBlocks now tolerates temp files #19458

superbobry commented Oct 9, 2017 •

edited

Loading

srowen Oct 9, 2017

superbobry Oct 9, 2017 •

edited

Loading

jiangxb1987 left a comment

jerryshao commented Oct 12, 2017 •

edited

Loading

superbobry commented Oct 12, 2017 •

edited

Loading

jerryshao commented Oct 12, 2017

superbobry commented Oct 12, 2017 •

edited

Loading

jerryshao Oct 13, 2017

cloud-fan Oct 13, 2017

superbobry Oct 13, 2017

cloud-fan Oct 16, 2017

superbobry Oct 23, 2017

jiangxb1987 commented Oct 19, 2017

cloud-fan Oct 23, 2017

cloud-fan Oct 23, 2017

superbobry Oct 24, 2017

cloud-fan commented Oct 24, 2017

SparkQA commented Oct 24, 2017

cloud-fan commented Oct 25, 2017

jerryshao commented Oct 25, 2017

SparkQA commented Oct 25, 2017

jerryshao commented Oct 25, 2017

SparkQA commented Oct 25, 2017

cloud-fan commented Oct 25, 2017

SparkQA commented Oct 25, 2017

cloud-fan commented Oct 25, 2017

[SPARK-22227][CORE] DiskBlockManager.getAllBlocks now tolerates temp files #19458

[SPARK-22227][CORE] DiskBlockManager.getAllBlocks now tolerates temp files #19458

Conversation

superbobry commented Oct 9, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

superbobry Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

jiangxb1987 left a comment

Choose a reason for hiding this comment

jerryshao commented Oct 12, 2017 • edited Loading

superbobry commented Oct 12, 2017 • edited Loading

jerryshao commented Oct 12, 2017

superbobry commented Oct 12, 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

Choose a reason for hiding this comment

jiangxb1987 commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Oct 24, 2017

SparkQA commented Oct 24, 2017

cloud-fan commented Oct 25, 2017

jerryshao commented Oct 25, 2017

SparkQA commented Oct 25, 2017

jerryshao commented Oct 25, 2017

SparkQA commented Oct 25, 2017

cloud-fan commented Oct 25, 2017

SparkQA commented Oct 25, 2017

cloud-fan commented Oct 25, 2017

superbobry commented Oct 9, 2017 •

edited

Loading

superbobry Oct 9, 2017 •

edited

Loading

jerryshao commented Oct 12, 2017 •

edited

Loading

superbobry commented Oct 12, 2017 •

edited

Loading

superbobry commented Oct 12, 2017 •

edited

Loading