Skip to content

Conversation

@WeichenXu123
Copy link
Contributor

@WeichenXu123 WeichenXu123 commented Sep 16, 2019

What changes were proposed in this pull request?

I fix the test "barrier task killed" which is flaky:

  • Split interrupt/no interrupt test into separate sparkContext. Prevent them to influence each other.
  • only check exception on partiton-0. partition-1 is hang on sleep which may throw other exception.

Why are the changes needed?

Make test robust.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

N/A

@WeichenXu123 WeichenXu123 changed the title [SPARK-28483][FOLLOW-UP] Fix flaky test in BarrierTaskSuite [SPARK-28483][FOLLOW-UP] Fix flaky test in BarrierTaskContextSuite Sep 16, 2019
@WeichenXu123
Copy link
Contributor Author

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 16, 2019

Test build #110634 has finished for PR 25799 at commit 9dbdcb8.

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

@SparkQA
Copy link

SparkQA commented Sep 16, 2019

Test build #4869 has finished for PR 25799 at commit 9dbdcb8.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Sep 17, 2019

Test build #110703 has finished for PR 25799 at commit 9dbdcb8.

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

@SparkQA
Copy link

SparkQA commented Sep 17, 2019

Test build #110732 has finished for PR 25799 at commit c8dcf42.

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

@WeichenXu123
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Sep 17, 2019

Test build #110739 has finished for PR 25799 at commit c8dcf42.

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

@dongjoon-hyun
Copy link
Member

Retest this please.

@SparkQA
Copy link

SparkQA commented Sep 17, 2019

Test build #110750 has finished for PR 25799 at commit c8dcf42.

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

@WeichenXu123 WeichenXu123 deleted the oss_fix_barrier_test branch September 17, 2019 11:10
@HyukjinKwon
Copy link
Member

HyukjinKwon commented Sep 17, 2019

@WeichenXu123, while this PR seems fine, please make sure you have a sign-off or an approval from another committer, which is a standard approach. At least, you can wait for some positive comments. Or, please clarify the reason why you're merging it without any sign-off from other people.

In this way, you can a) avoid unexpected mistakes, and b) have another person who can take over your change, say, if you happen to leave this project; otherwise, no one can take over if most of changes are just merged without peer-review.

Although it is legitimate to merge as a committer as far as I know, we should better stick to this practice. Avoid to set a bad example.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Sep 17, 2019

Also, please clarify which branch you merged (e.g., "Merged to master") to avoid the overhead for people to check commit history to see which branch it was merged.

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Looks OK to me.
Yeah I'd say it's a judgment call, if it's a minor change and you feel sure about it and nobody has weighed in for a day or two, it's not unreasonable to proceed. If in doubt I'd try pinging someone for a second look.

@srowen
Copy link
Member

srowen commented Sep 17, 2019

For the record looks like this was merged to master

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants