-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-31784][CORE][TEST] Fix test BarrierTaskContextSuite."share messages with allGather() call" #28596
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.
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
Conversation
|
Test build #122920 has finished for PR 28596 at commit
|
jiangxb1987
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
retest this please |
|
Test build #122949 has finished for PR 28596 at commit
|
|
retest this please |
| val messages = rdd2.collect().head | ||
| // All the task partitionIds are shared | ||
| for((x, i) <- messages.view.zipWithIndex) assert(x.toString == i.toString) | ||
| assert(messages === List("0", "1", "2", "3")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd better verify the whole rdd collect array, not only the head of result array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense.
|
Test build #122959 has finished for PR 28596 at commit
|
|
LGTM |
…sages with allGather() call"
### What changes were proposed in this pull request?
Change from `messages.toList.iterator` to `Iterator.single(messages.toList)`.
### Why are the changes needed?
In this test, the expected result of `rdd2.collect().head` should actually be `List("0", "1", "2", "3")` but is `"0"` now.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Updated test.
Thanks WeichenXu123 reported this problem.
Closes #28596 from Ngone51/fix_allgather_test.
Authored-by: yi.wu <[email protected]>
Signed-off-by: Xingbo Jiang <[email protected]>
(cherry picked from commit 83d0967)
Signed-off-by: Xingbo Jiang <[email protected]>
|
Thanks, merged to master/3.0 ! |
|
Test build #122962 has finished for PR 28596 at commit
|
|
thanks all! |
What changes were proposed in this pull request?
Change from
messages.toList.iteratortoIterator.single(messages.toList).Why are the changes needed?
In this test, the expected result of
rdd2.collect().headshould actually beList("0", "1", "2", "3")but is"0"now.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Updated test.
Thanks @WeichenXu123 reported this problem.