Skip to content

[SPARK-20333] HashPartitioner should be compatible with num of child RDD's partitions.#17634

Closed
jinxing64 wants to merge 2 commits intoapache:masterfrom
jinxing64:SPARK-20333
Closed

[SPARK-20333] HashPartitioner should be compatible with num of child RDD's partitions.#17634
jinxing64 wants to merge 2 commits intoapache:masterfrom
jinxing64:SPARK-20333

Conversation

@jinxing64
Copy link
Copy Markdown

@jinxing64 jinxing64 commented Apr 14, 2017

What changes were proposed in this pull request?

Fix test
"don't submit stage until its dependencies map outputs are registered (SPARK-5259)" ,
"run trivial shuffle with out-of-band executor failure and retry",
"reduce tasks should be placed locally with map output"
in DAGSchedulerSuite.

@jinxing64
Copy link
Copy Markdown
Author

jinxing64 commented Apr 14, 2017

I found this when do #17533

@SparkQA
Copy link
Copy Markdown

SparkQA commented Apr 14, 2017

Test build #75787 has finished for PR 17634 at commit e9d8297.

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

@SparkQA
Copy link
Copy Markdown

SparkQA commented Apr 14, 2017

Test build #75788 has finished for PR 17634 at commit cb80dbd.

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

@jinxing64
Copy link
Copy Markdown
Author

@squito @srowen
Could you help comment on this :)

@jinxing64
Copy link
Copy Markdown
Author

@kayousterhout @mridulm
Does this pr make sense?
Could you please take a look this when you have time :)

@jiangxb1987
Copy link
Copy Markdown
Contributor

jiangxb1987 commented May 23, 2017

The test cases are not falling on master branch, are there some good reasons to change them?

@jinxing64
Copy link
Copy Markdown
Author

@jiangxb1987
Thank you so much taking time looking into this.
Yes, it is not failing in existing code. But I think it's quite straightforward that partitioner should be compatible with num of child RDD's partitions, right?
In #17533, I have to query the MapStatus based on the correctness of partitioner(i.e. num of partitions). That's why I proposed this.
Thanks again taking time reviewing this :) 👍

@jiangxb1987
Copy link
Copy Markdown
Contributor

Don't think we could fail on the original test case for any reason, otherwise the legacy user code could fail unexpectedly, isn't it?

@jinxing64
Copy link
Copy Markdown
Author

Yes. it doesn't fail for sure.
I just think it's fairly straightforward that partitioner should be compatible with num of child RDD's partitions. I find no reason the num of partitions are not equal in those test.

@squito
Copy link
Copy Markdown
Contributor

squito commented May 30, 2017

lgtm.
sorry for such a late review. Since its been a while I'll trigger tests again just to be safe before merging.

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 30, 2017

Test build #3767 has finished for PR 17634 at commit cb80dbd.

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

@squito
Copy link
Copy Markdown
Contributor

squito commented May 30, 2017

merged to master

@asfgit asfgit closed this in de953c2 May 30, 2017
@jinxing64
Copy link
Copy Markdown
Author

@squito
Thanks a lot for merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants