[SPARK-31034][CORE] ShuffleBlockFetcherIterator should always create request for last block group #27786

Ngone51 · 2020-03-04T07:27:12Z

What changes were proposed in this pull request?

This is a bug fix of #27280. This PR fix the bug where ShuffleBlockFetcherIterator may forget to create request for the last block group.

Why are the changes needed?

When (all blocks).sum < targetRemoteRequestSize and (all blocks).length > maxBlocksInFlightPerAddress and (last block group).size < maxBlocksInFlightPerAddress,
ShuffleBlockFetcherIterator will not create a request for the last group. Thus, it will lost data for the reduce task.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Updated test.

Ngone51 · 2020-03-04T07:28:06Z

cc @cloud-fan @xuanyuanking

cloud-fan · 2020-03-04T07:54:30Z

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

@@ -339,14 +339,14 @@ final class ShuffleBlockFetcherIterator(
        + s"with ${blocks.size} blocks")
    }

-    def createFetchRequests(): Unit = {
+    def createFetchRequests(hasMore: Boolean): Unit = {


nit: isLast?

cloud-fan · 2020-03-04T07:54:49Z

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

@@ -367,12 +367,12 @@ final class ShuffleBlockFetcherIterator(
      // For batch fetch, the actual block in flight should count for merged block.
      val mayExceedsMaxBlocks = !doBatchFetch && curBlocks.size >= maxBlocksInFlightPerAddress
      if (curRequestSize >= targetRemoteRequestSize || mayExceedsMaxBlocks) {
-        createFetchRequests()
+        createFetchRequests(true)


let's write down the parameter name.

SparkQA · 2020-03-04T08:05:02Z

Test build #119285 has finished for PR 27786 at commit fc36eb1.

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

cloud-fan · 2020-03-04T08:54:52Z

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

+
+    var numResults = 0
+    // After initialize(), there will be 6 FetchRequests, and the each of the first 5
+    // includes 3 merged blocks and the last one has 1 merged block. So, only the


there are 6 merged blocks in total, how can each request includes 3 merged blocks?

or do you mean shuffle blocks?

Yes, it's shuffle blocks in this case. But it seems inconsistent with comment in the below test....I need reword it.

note: 3 merged blocks -> 3 shuffle block(not batch)

ok let's update

Ngone51 · 2020-03-04T09:02:31Z

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

    var numResults = 0
+    // After initialize(), there will be 2 FetchRequests that one has 2 merged blocks and another


note: 2 merged blocks -> 2 ShuffleBlockBatch

SparkQA · 2020-03-04T12:02:08Z

Test build #119294 has finished for PR 27786 at commit 778be33.

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

SparkQA · 2020-03-04T12:12:57Z

Test build #119287 has finished for PR 27786 at commit 28df4ab.

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

cloud-fan · 2020-03-04T14:54:15Z

core/src/test/scala/org/apache/spark/storage/ShuffleBlockFetcherIteratorSuite.scala

+    // After initialize(), there will be 6 FetchRequests. And each of the first 5 requests
+    // includes 1 merged block which is merged from 3 shuffle blocks. The last request has 1 merged
+    // block which merged from 2 shuffle blocks. So, only the first 5 requests(5 * 3 * 100 >= 1500)
+    // can be sent. The second FetchRequest will hit maxBlocksInFlightPerAddress so it won't


The second -> The 6th?

SparkQA · 2020-03-04T16:38:50Z

Test build #119308 has finished for PR 27786 at commit 6766c7c.

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

SparkQA · 2020-03-04T19:46:00Z

Test build #119319 has finished for PR 27786 at commit dc94365.

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

cloud-fan · 2020-03-05T13:31:37Z

thanks, merging to master/3.0!

…request for last block group ### What changes were proposed in this pull request? This is a bug fix of #27280. This PR fix the bug where `ShuffleBlockFetcherIterator` may forget to create request for the last block group. ### Why are the changes needed? When (all blocks).sum < `targetRemoteRequestSize` and (all blocks).length > `maxBlocksInFlightPerAddress` and (last block group).size < `maxBlocksInFlightPerAddress`, `ShuffleBlockFetcherIterator` will not create a request for the last group. Thus, it will lost data for the reduce task. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Updated test. Closes #27786 from Ngone51/fix_no_request_bug. Authored-by: yi.wu <yi.wu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 2257ce2) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Ngone51 · 2020-03-05T15:07:33Z

thanks all!

…request for last block group ### What changes were proposed in this pull request? This is a bug fix of apache#27280. This PR fix the bug where `ShuffleBlockFetcherIterator` may forget to create request for the last block group. ### Why are the changes needed? When (all blocks).sum < `targetRemoteRequestSize` and (all blocks).length > `maxBlocksInFlightPerAddress` and (last block group).size < `maxBlocksInFlightPerAddress`, `ShuffleBlockFetcherIterator` will not create a request for the last group. Thus, it will lost data for the reduce task. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Updated test. Closes apache#27786 from Ngone51/fix_no_request_bug. Authored-by: yi.wu <yi.wu@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

fix

fc36eb1

Ngone51 mentioned this pull request Mar 4, 2020

[SPARK-31017][TEST][CORE] Test for shuffle requests packaging with different size and numBlocks limit #27767

Closed

cloud-fan reviewed Mar 4, 2020

View reviewed changes

Ngone51 added 4 commits March 4, 2020 16:10

address comment

2125213

add test

28df4ab

add nother test

313637b

new line

778be33

cloud-fan reviewed Mar 4, 2020

View reviewed changes

Ngone51 commented Mar 4, 2020

View reviewed changes

update comment

6766c7c

cloud-fan reviewed Mar 4, 2020

View reviewed changes

cloud-fan approved these changes Mar 4, 2020

View reviewed changes

address comment

dc94365

cloud-fan closed this in 2257ce2 Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-31034][CORE] ShuffleBlockFetcherIterator should always create request for last block group #27786

[SPARK-31034][CORE] ShuffleBlockFetcherIterator should always create request for last block group #27786

Ngone51 commented Mar 4, 2020 •

edited

Ngone51 commented Mar 4, 2020

cloud-fan Mar 4, 2020

cloud-fan Mar 4, 2020

SparkQA commented Mar 4, 2020

cloud-fan Mar 4, 2020

cloud-fan Mar 4, 2020

Ngone51 Mar 4, 2020

Ngone51 Mar 4, 2020

cloud-fan Mar 4, 2020

Ngone51 Mar 4, 2020

SparkQA commented Mar 4, 2020

SparkQA commented Mar 4, 2020

cloud-fan Mar 4, 2020

SparkQA commented Mar 4, 2020

SparkQA commented Mar 4, 2020

cloud-fan commented Mar 5, 2020

Ngone51 commented Mar 5, 2020

		var numResults = 0
		// After initialize(), there will be 2 FetchRequests that one has 2 merged blocks and another

[SPARK-31034][CORE] ShuffleBlockFetcherIterator should always create request for last block group #27786

[SPARK-31034][CORE] ShuffleBlockFetcherIterator should always create request for last block group #27786

Conversation

Ngone51 commented Mar 4, 2020 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Ngone51 commented Mar 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 4, 2020

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

Choose a reason for hiding this comment

SparkQA commented Mar 4, 2020

SparkQA commented Mar 4, 2020

Choose a reason for hiding this comment

SparkQA commented Mar 4, 2020

SparkQA commented Mar 4, 2020

cloud-fan commented Mar 5, 2020

Ngone51 commented Mar 5, 2020

Ngone51 commented Mar 4, 2020 •

edited