[SPARK-31226][CORE][TESTS] SizeBasedCoalesce logic will lose partition #27988

AngersZhuuuu · 2020-03-23T09:13:47Z

What changes were proposed in this pull request?

When last partition's splitFile's split size is larger then maxSize, this partition will be lost

Origin logic error like below as 1, 2, 3, 4, 5

// 5. since index = partition.size now,  jump out of the loop , then the last partition is lost since we won't call updatePartition() again.
while (index < partitions.size) {
     //  1. we assume that when index = partitions.length -1(the last partition) 
      val partition = partitions(index)
      val fileSplit =
        partition.asInstanceOf[HadoopPartition].inputSplit.value.asInstanceOf[FileSplit]
      val splitSize = fileSplit.getLength
     // 2.  if  this partition's  splitSize > maxSize
      if (currentSum + splitSize < maxSize) {
        addPartition(partition, splitSize)
        index += 1
        if (index == partitions.size) {
          updateGroups
        }
      } else {
       //  3. if currentGroup.partitions.size  >0, this situation is possiable
        if (currentGroup.partitions.size == 0) {
          addPartition(partition, splitSize)
          index += 1
        } else {
        //   4. then will just call updateGroups() here first, and index won't update in group
          updateGroups
        }
      }
    }
    groups.toArray
  }
}

Why are the changes needed?

Fix bug

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Manual code review.

dongjoon-hyun · 2020-04-01T19:31:24Z

ok to test

dongjoon-hyun · 2020-04-01T19:33:27Z

Sorry for late visiting here, @AngersZhuuuu .
BTW, please use PR description instead of comment (#27988 (comment)).

The PR description will be a commit log with your patch.
A comment on PR is hard to find later.

SparkQA · 2020-04-01T19:36:28Z

Test build #120689 has finished for PR 27988 at commit 2c4ef14.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-04-01T19:36:29Z

core/src/test/scala/org/apache/spark/rdd/RDDSuite.scala

+      index += 1
+      if (index == partitions.length) {
+        updateGroups()
+      }


BTW, the previous code was added at 2.0.0. Did you hit a test flakiness due to this?

BTW, the previous code was added at 2.0.0. Did you hit a test flakiness due to this?

Yeah, these days I am testing a way to fix spark small out put file problem, and I use this logic, during test, found that this logic is wrong.
#27248

dongjoon-hyun · 2020-04-01T19:38:51Z

Retest this please.

SparkQA · 2020-04-01T22:54:04Z

Test build #120690 has finished for PR 27988 at commit 2c4ef14.

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

AngersZhuuuu · 2020-04-02T02:15:33Z

Sorry for late visiting here, @AngersZhuuuu .
BTW, please use PR description instead of comment (#27988 (comment)).

The PR description will be a commit log with your patch.

A comment on PR is hard to find later.

Changed

AngersZhuuuu · 2020-04-06T13:24:22Z

@dongjoon-hyun Any more need to add for this?

cloud-fan · 2020-07-09T12:30:27Z

core/src/test/scala/org/apache/spark/rdd/RDDSuite.scala

          addPartition(partition, splitSize)
-          index += 1
        } else {
-          updateGroups


can you point out which line/lines cause the bug?

can you point out which line/lines cause the bug?

In the pr description I have show the case this bug happen. Not one line or some lines cause this error.

can you point out which line/lines cause the bug?

Change the desc make it more clear.

cloud-fan · 2020-07-10T07:23:32Z

core/src/test/scala/org/apache/spark/rdd/RDDSuite.scala

      }
+      index += 1
+      if (index == partitions.length) {
+        updateGroups()


nit: can we do this after the loop exit? There is no early stop in this loop.

nit: can we do this after the loop exit? There is no early stop in this loop.

Sure, updated, since before exit loop, the last action is addPartition so currentGourps if not empty, we don't need to check if currentGroup is empty, just updateGroups

SparkQA · 2020-07-10T12:40:32Z

Test build #125580 has finished for PR 27988 at commit d7764ac.

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

cloud-fan · 2020-07-10T13:39:08Z

retest this please

SparkQA · 2020-07-10T18:25:14Z

Test build #125615 has finished for PR 27988 at commit d7764ac.

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

AngersZhuuuu · 2020-07-10T23:07:19Z

@cloud-fan The test failed is not related since only test RDDSuite.custom RDD coalescer will use this SizeBasedCoalescer

AngersZhuuuu · 2020-07-10T23:07:33Z

retest this please

dongjoon-hyun · 2020-07-10T23:35:47Z

core/src/test/scala/org/apache/spark/rdd/RDDSuite.scala

-          updateGroups
+          updateGroups()
+          addPartition(partition, splitSize)
        }


Instead of the following,

if (currentGroup.partitions.isEmpty) { addPartition(partition, splitSize) } else { updateGroups() addPartition(partition, splitSize) }

The following will be better.

if (currentGroup.partitions.nonEmpty) { updateGroups() } addPartition(partition, splitSize)

Updated, thanks

dongjoon-hyun

+1, LGTM (except one minor refactoring comment)

SparkQA · 2020-07-11T04:06:10Z

Test build #125650 has finished for PR 27988 at commit d7764ac.

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

SparkQA · 2020-07-11T04:19:16Z

Test build #125653 has finished for PR 27988 at commit 20b1183.

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

AngersZhuuuu · 2020-07-11T04:24:46Z

retest this please

SparkQA · 2020-07-11T07:05:02Z

Test build #125665 has finished for PR 27988 at commit 20b1183.

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

maropu · 2020-07-11T08:02:47Z

retest this please

SparkQA · 2020-07-11T11:12:26Z

Test build #125670 has finished for PR 27988 at commit 20b1183.

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

dongjoon-hyun

+1, LGTM. Merged to master. Thanks, @AngersZhuuuu .

AngersZhuuuu added 2 commits March 23, 2020 17:10

Update RDDSuite.scala

007f49c

Update RDDSuite.scala

2c4ef14

dongjoon-hyun added the TESTS label Apr 1, 2020

dongjoon-hyun reviewed Apr 1, 2020

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-31226][SQL][TEST] SizeBasedCoalesce logic will lose partition~~ [SPARK-31226][CORE][TEST] SizeBasedCoalesce logic will lose partition Apr 1, 2020

dongjoon-hyun added the SPARK CORE label Apr 1, 2020

cloud-fan reviewed Jul 9, 2020

View reviewed changes

cloud-fan reviewed Jul 10, 2020

View reviewed changes

Update RDDSuite.scala

d7764ac

probot-autolabeler bot added the CORE label Jul 10, 2020

cloud-fan approved these changes Jul 10, 2020

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-31226][CORE][TEST] SizeBasedCoalesce logic will lose partition~~ [SPARK-31226][CORE][TESTS] SizeBasedCoalesce logic will lose partition Jul 10, 2020

dongjoon-hyun reviewed Jul 10, 2020

View reviewed changes

dongjoon-hyun approved these changes Jul 10, 2020

View reviewed changes

Update RDDSuite.scala

20b1183

dongjoon-hyun approved these changes Jul 11, 2020

View reviewed changes

dongjoon-hyun closed this in 09789ff Jul 11, 2020

[SPARK-31226][CORE][TESTS] SizeBasedCoalesce logic will lose partition #27988

[SPARK-31226][CORE][TESTS] SizeBasedCoalesce logic will lose partition #27988

Uh oh!

Conversation

AngersZhuuuu commented Mar 23, 2020 • edited by dongjoon-hyun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

dongjoon-hyun commented Apr 1, 2020

Uh oh!

dongjoon-hyun commented Apr 1, 2020

Uh oh!

SparkQA commented Apr 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Apr 1, 2020

Uh oh!

SparkQA commented Apr 1, 2020

Uh oh!

AngersZhuuuu commented Apr 2, 2020

Uh oh!

AngersZhuuuu commented Apr 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

cloud-fan commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

AngersZhuuuu commented Jul 10, 2020

Uh oh!

AngersZhuuuu commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 11, 2020

Uh oh!

SparkQA commented Jul 11, 2020

Uh oh!

AngersZhuuuu commented Jul 11, 2020

Uh oh!

SparkQA commented Jul 11, 2020

Uh oh!

maropu commented Jul 11, 2020

Uh oh!

SparkQA commented Jul 11, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

AngersZhuuuu commented Mar 23, 2020 •

edited by dongjoon-hyun

Loading

AngersZhuuuu Apr 2, 2020 •

edited

Loading