Skip to content

[SPARK-32055][CORE][SQL] Unify getReader and getReaderForRange in ShuffleManager #28895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 15 commits into from
Closed

[SPARK-32055][CORE][SQL] Unify getReader and getReaderForRange in ShuffleManager #28895

wants to merge 15 commits into from

Conversation

@Ngone51
Copy link
Member

@Ngone51 Ngone51 commented Jun 22, 2020

What changes were proposed in this pull request?

This PR tries to unify the method getReader and getReaderForRange in ShuffleManager.

Why are the changes needed?

Reduce the duplicate codes, simplify the implementation, and for better maintenance.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Covered by existing tests.

@SparkQA
Copy link

SparkQA commented Jun 22, 2020

Test build #124368 has finished for PR 28895 at commit f2d28dd.

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

dongjoon-hyun
dongjoon-hyun reviewed Jun 23, 2020
View reviewed changes
core/src/main/scala/org/apache/spark/MapOutputTracker.scala Outdated
def getMapSizesByExecutorId(shuffleId: Int, reduceId: Int)
: Iterator[(BlockManagerId, Seq[(BlockId, Long, Int)])] = {
getMapSizesByExecutorId(shuffleId, reduceId, reduceId + 1)
getMapSizesByExecutorId(shuffleId, mapStatus => (0, mapStatus.length), reduceId, reduceId + 1)
Copy link
Member

@dongjoon-hyun dongjoon-hyun Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since mapStatus => (0, mapStatus.length) seems to be used frequently, do you think we can define this function somewhere to prevent mistakes?

Copy link
Member Author

@Ngone51 Ngone51 Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sounds good.

@Ngone51
Copy link
Member Author

Ngone51 commented Jun 23, 2020

@JkSelf @cloud-fan Could you please take a look? Thanks!

@SparkQA
Copy link

SparkQA commented Jun 23, 2020

Test build #124376 has finished for PR 28895 at commit 05fe4c7.

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

@Ngone51
Copy link
Member Author

Ngone51 commented Jun 23, 2020

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Jun 23, 2020

Test build #124377 has finished for PR 28895 at commit 05fe4c7.

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

@Ngone51
Copy link
Member Author

Ngone51 commented Jun 23, 2020

Jenkins, retest this please.

cloud-fan
cloud-fan reviewed Jun 23, 2020
View reviewed changes
JkSelf
JkSelf reviewed Jun 23, 2020
View reviewed changes
core/src/main/scala/org/apache/spark/shuffle/ShuffleManager.scala Outdated
handle: ShuffleHandle,
startMapIndex: Int,
endMapIndex: Int,
mapIndexRange: Array[MapStatus] => (Int, Int),
Copy link
Contributor

@JkSelf JkSelf Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with the parameters of startPartition and endPartition, here we can remain the startMapIndex and endMapIndex in getReader method. It will be more clearly.

Copy link
Member Author

@Ngone51 Ngone51 Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is we can not know the endMapIndex at the caller side. Maybe we could try @cloud-fan 's suggestion above.

@SparkQA
Copy link

SparkQA commented Jun 23, 2020

Test build #124380 has finished for PR 28895 at commit 05fe4c7.

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

cloud-fan
cloud-fan reviewed Jun 23, 2020
View reviewed changes
cloud-fan
cloud-fan reviewed Jun 23, 2020
View reviewed changes
cloud-fan
cloud-fan reviewed Jun 23, 2020
View reviewed changes
cloud-fan
cloud-fan reviewed Jun 23, 2020
View reviewed changes
cloud-fan
cloud-fan reviewed Jun 23, 2020
View reviewed changes
cloud-fan
cloud-fan reviewed Jun 23, 2020
View reviewed changes
cloud-fan
cloud-fan reviewed Jun 23, 2020
View reviewed changes
cloud-fan
cloud-fan reviewed Jun 23, 2020
View reviewed changes
@SparkQA
Copy link

SparkQA commented Jun 23, 2020

Test build #124393 has finished for PR 28895 at commit 2a31450.

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

@SparkQA
Copy link

SparkQA commented Jun 23, 2020

Test build #124411 has finished for PR 28895 at commit 109eeb3.

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

cloud-fan
cloud-fan reviewed Jun 23, 2020
View reviewed changes
@SparkQA
Copy link

SparkQA commented Jun 23, 2020

Test build #124419 has finished for PR 28895 at commit b7ec583.

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

@SparkQA
Copy link

SparkQA commented Jun 24, 2020

Test build #124464 has finished for PR 28895 at commit 2994050.

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

@Ngone51
Copy link
Member Author

Ngone51 commented Jun 24, 2020

retest this please.

@SparkQA
Copy link

SparkQA commented Jun 24, 2020

Test build #124465 has finished for PR 28895 at commit 2994050.

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

@Ngone51
Copy link
Member Author

Ngone51 commented Jun 24, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Jun 24, 2020

Test build #124487 has finished for PR 28895 at commit 2994050.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Jun 24, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Jun 24, 2020

Test build #124494 has finished for PR 28895 at commit 2994050.

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

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jun 27, 2020

Retest this please.

@SparkQA
Copy link

SparkQA commented Jun 27, 2020

Test build #124555 has finished for PR 28895 at commit 2994050.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Jun 29, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Jun 29, 2020

Test build #5048 has finished for PR 28895 at commit 4247fa3.

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

@Ngone51
Copy link
Member Author

Ngone51 commented Jun 29, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Jun 29, 2020

Test build #124623 has finished for PR 28895 at commit 4247fa3.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Jun 29, 2020

thanks, merging to master!

@cloud-fan cloud-fan closed this in 6fcb70e Jun 29, 2020
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jun 29, 2020

+1, late LGTM. Thank you, @Ngone51 and @cloud-fan .

@SparkQA
Copy link

SparkQA commented Jun 29, 2020

Test build #124630 has finished for PR 28895 at commit 4247fa3.

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

@Ngone51
Copy link
Member Author

Ngone51 commented Jun 29, 2020

thanks all!

wangyum pushed a commit that referenced this pull request May 26, 2023
@Ngone51
[SPARK-32055][CORE][SQL] Unify getReader and getReaderForRange in Shu…
573345a 
…ffleManager

### What changes were proposed in this pull request?

This PR tries to unify the method `getReader` and `getReaderForRange` in `ShuffleManager`.

### Why are the changes needed?

Reduce the duplicate codes, simplify the implementation, and for better maintenance.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Covered by existing tests.

Closes #28895 from Ngone51/unify-getreader.

Authored-by: yi.wu <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

@cloud-fan cloud-fan cloud-fan approved these changes

+1 more reviewer

@JkSelf JkSelf JkSelf left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CORE SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Ngone51 @SparkQA @cloud-fan @dongjoon-hyun @JkSelf