[SPARK-23524] Big local shuffle blocks should not be checked for corruption. by jinxing64 · Pull Request #20685 · apache/spark

jinxing64 · 2018-02-27T10:03:02Z

What changes were proposed in this pull request?

In current code, all local blocks will be checked for corruption no matter it's big or not. The reasons are as below:

Size in FetchResult for local block is set to be 0 (https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala#L327)
SPARK-4105 meant to only check the small blocks(size<maxBytesInFlight/3), but for reason 1, below check will be invalid. https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala#L420

We can fix this and avoid the OOM.

How was this patch tested?

UT added

jinxing64 · 2018-02-27T10:03:20Z

Jenkins, test this please

SparkQA · 2018-02-27T13:09:07Z

Test build #87714 has finished for PR 20685 at commit 535916c.

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

…uption.

SparkQA · 2018-02-28T05:20:40Z

Test build #87741 has finished for PR 20685 at commit 110c851.

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

jinxing64 · 2018-02-28T06:19:43Z

Jenkins, retest this please

SparkQA · 2018-02-28T08:05:02Z

Test build #87755 has finished for PR 20685 at commit 110c851.

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

jinxing64 · 2018-02-28T11:00:16Z

Jenkins, retest this please.

SparkQA · 2018-02-28T14:05:32Z

Test build #87770 has finished for PR 20685 at commit 110c851.

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

jinxing64 · 2018-03-01T01:48:06Z

The failed test FileBasedDataSourceSuite is not related， I can pass in my local

jinxing64 · 2018-03-01T01:49:49Z

Jenkins, retest this please.

SparkQA · 2018-03-01T05:42:56Z

Test build #87807 has finished for PR 20685 at commit 110c851.

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

jinxing64 · 2018-03-01T06:12:51Z

cc @cloud-fan @zsxwing @squito @jiangxb1987
Could you please help take a look?

squito

minor cleanup in the test, otherwise lgtm. thanks for catching this and suggesting a fix.

squito · 2018-03-05T15:37:50Z

 intercept[FetchFailedException] { iterator.next() }
 }

+ test("big corrupt blocks will not be retiried") {


typo: retried (or maybe "retired", not sure)
though I think a better name would be "big blocks are not checked for corruption"

I will refine this.

squito · 2018-03-05T15:54:02Z

+ )
+
+ val transfer = mock(classOf[BlockTransferService])
+ when(transfer.fetchBlocks(any(), any(), any(), any(), any(), any()))


you can reuse createMockTransfer to simplify this a little.
(actually, a bunch of this test code looks like it could be refactored across these tests -- but we can leave that out of this change.)

Thanks a lot~ Imran, I can file another pr for the refine :)

sorry my comment was vague -- I do think you can use createMockTransfer here, since that helper method already exists.
I was just thinking that there may be more we could clean up -- setting up the local & remote BlockManager Id, creating the ShuffleIterator, etc. seems to have a lot of boilerplate in all the tests. But let's not to do a pure refactoring to the other tests in this change.

cloud-fan · 2018-03-05T22:36:29Z

We should update the doc of SuccessFetchResult#size, currently it's saying estimated size of the block, used to calculate bytesInFlight.

jinxing64 · 2018-03-06T11:56:24Z

@squito @cloud-fan
Thanks you so much for reviewing. I refined accordingly. Please take another look when you have time.

SparkQA · 2018-03-06T15:56:13Z

Test build #88003 has finished for PR 20685 at commit 1cd8653.

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

cloud-fan · 2018-03-06T21:06:33Z

From what I've seen in #15923 , this corruption check is mostly to detect network failures, so seems like we don't need this check for local blocks at all.

cc @davies @zsxwing

squito · 2018-03-07T02:48:26Z

it'll also help with disk corruption ... from the stack traces in SPARK-4105 you can't really tell what the source of the problem is. it'll be pretty hard to determine what the source of corruption is if we start seeing it again. anyway, I don't feel that strongly about it either way.

Ngone51 · 2018-03-07T03:15:43Z

- * @param size estimated size of the block, used to calculate bytesInFlight.
- *  Note that this is NOT the exact bytes.
+ * @param size estimated size of the block. Note that this is NOT the exact bytes.
+* Size of remote block is used to calculate bytesInFlight.


nit: documentation style

cloud-fan · 2018-03-07T05:06:59Z

sounds reasonable. The purpose of this corruption check is to fail fast to retry the stage(re-shuffle), so disk corruption should also be counted.

cloud-fan · 2018-03-07T05:11:44Z

LGTM

jinxing64 · 2018-03-07T05:25:29Z

@cloud-fan @squito @Ngone51
Thanks a lot !

SparkQA · 2018-03-07T07:12:39Z

Test build #88033 has finished for PR 20685 at commit 4e4f075.

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

zsxwing · 2018-03-07T08:13:52Z

I think #20179 probably already fixed the data corruption issue.

cloud-fan · 2018-03-07T23:11:41Z

yea very likely, but I'm not 100% sure. How about we merge this one first to fix the mistake for local shuffle blocks, and then think about whether or not we should remove this corruption check?

squito · 2018-03-08T03:25:38Z

I agree with @cloud-fan .

squito · 2018-03-08T03:26:17Z

lgtm

cloud-fan · 2018-03-08T04:09:19Z

thanks, merging to master/2.3!

…uption. ## What changes were proposed in this pull request? In current code, all local blocks will be checked for corruption no matter it's big or not. The reasons are as below: Size in FetchResult for local block is set to be 0 (https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala#L327) SPARK-4105 meant to only check the small blocks(size<maxBytesInFlight/3), but for reason 1, below check will be invalid. https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala#L420 We can fix this and avoid the OOM. ## How was this patch tested? UT added Author: jx158167 <jx158167@antfin.com> Closes#20685 from jinxing64/SPARK-23524. (cherry picked from commit 77c91cc) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

jinxing64 · 2018-03-08T05:21:14Z

Thanks for merging !
@cloud-fan @squito @zsxwing @Ngone51

…uption. ## What changes were proposed in this pull request? In current code, all local blocks will be checked for corruption no matter it's big or not. The reasons are as below: Size in FetchResult for local block is set to be 0 (https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala#L327) SPARK-4105 meant to only check the small blocks(size<maxBytesInFlight/3), but for reason 1, below check will be invalid. https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/ShuffleBlockFetcherIterator.scala#L420 We can fix this and avoid the OOM. ## How was this patch tested? UT added Author: jx158167 <jx158167@antfin.com> Closesapache#20685 from jinxing64/SPARK-23524. (cherry picked from commit 77c91cc) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

[SPARK-23524] Big local shuffle blocks should not be checked for corr…
110c851
…uption.

jinxing64 force-pushed the SPARK-23524 branch from 535916c to 110c851Compare February 28, 2018 02:21

squito approved these changes Mar 5, 2018
View reviewed changes

resolve doc and test's name
1cd8653

Ngone51 reviewed Mar 7, 2018
View reviewed changes

refine test
4e4f075

jinxing64 force-pushed the SPARK-23524 branch from 5ec3687 to 4e4f075Compare March 7, 2018 03:20

asfgit closed this in 77c91ccMar 8, 2018

Uh oh!

Conversation

jinxing64 commented Feb 27, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

jinxing64 commented Feb 27, 2018

Uh oh!

SparkQA commented Feb 27, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

jinxing64 commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

jinxing64 commented Feb 28, 2018

Uh oh!

SparkQA commented Feb 28, 2018

Uh oh!

jinxing64 commented Mar 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jinxing64 commented Mar 1, 2018

Uh oh!

SparkQA commented Mar 1, 2018

Uh oh!

jinxing64 commented Mar 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

squito left a comment

Choose a reason for hiding this comment

Uh oh!

squitoMar 5, 2018

Choose a reason for hiding this comment

Uh oh!

jinxing64Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

squitoMar 5, 2018

Choose a reason for hiding this comment

Uh oh!

jinxing64Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

squitoMar 6, 2018

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Mar 5, 2018

Uh oh!

jinxing64 commented Mar 6, 2018

Uh oh!

SparkQA commented Mar 6, 2018

Uh oh!

cloud-fan commented Mar 6, 2018

Uh oh!

squito commented Mar 7, 2018

Uh oh!

Ngone51Mar 7, 2018

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Mar 7, 2018

Uh oh!

cloud-fan commented Mar 7, 2018

Uh oh!

jinxing64 commented Mar 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Mar 7, 2018

Uh oh!

zsxwing commented Mar 7, 2018

Uh oh!

cloud-fan commented Mar 7, 2018

Uh oh!

squito commented Mar 8, 2018

Uh oh!

squito commented Mar 8, 2018

Uh oh!

cloud-fan commented Mar 8, 2018

Uh oh!

jinxing64 commented Mar 1, 2018•
edited
Loading

jinxing64 commented Mar 1, 2018•
edited
Loading

jinxing64 commented Mar 7, 2018•
edited
Loading