Skip to content
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

[SPARK-4169] [Core] Accommodate non-English Locales in unit tests #3036

Closed
wants to merge 1 commit into from
Closed

[SPARK-4169] [Core] Accommodate non-English Locales in unit tests #3036

wants to merge 1 commit into from

Conversation

numbnut
Copy link

@numbnut numbnut commented Oct 31, 2014

For me the core tests failed because there are two locale dependent parts in the code.
Look at the Jira ticket for details.

Why is it necessary to check the exception message in isBindCollision in
https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/util/Utils.scala#L1686
?

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@@ -1683,7 +1683,7 @@ private[spark] object Utils extends Logging {
def isBindCollision(exception: Throwable): Boolean = {
exception match {
case e: BindException =>
if (e.getMessage != null && e.getMessage.contains("Address already in use")) {
if (e.getMessage != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's hacky to depend on the exact message. I suppose this will now trigger on any BindException with a message, which could include things like invalid port or other failure. But maybe that's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's probably not a whole lot of harm in a false-positive here, since I think we limit the number of retries and will eventually fail.

@numbnut numbnut changed the title [Core] Locale dependent code [SPARK-4169] [Core] Accommodate non-English Locales in unit tests Oct 31, 2014
@JoshRosen JoshRosen mentioned this pull request Nov 7, 2014
@JoshRosen
Copy link
Contributor

Jenkins, this is ok to test.

@SparkQA
Copy link

SparkQA commented Nov 8, 2014

Test build #23085 has started for PR 3036 at commit 1fb0d04.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Nov 8, 2014

Test build #23085 has finished for PR 3036 at commit 1fb0d04.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23085/
Test PASSed.

@JoshRosen
Copy link
Contributor

/cc @andrewor14 Any thoughts here? Some other users have started hitting this, too, so I think this is a high priority patch to merge.

@JoshRosen
Copy link
Contributor

This doesn't just affect unit tests, which is why it's important: http://issues.apache.org/jira/browse/SPARK-4316

@andrewor14
Copy link
Contributor

The reason why it searches for the specific message is that we want to isolate the specific kind of BindException that corresponds to port collision. The issue is that there are other kinds of BindExceptions that we cannot get around by incrementing the port, e.g. permission denied. This patch will cause some false positives, but I think it's worth merging it because what it fixes is more important.

@andrewor14
Copy link
Contributor

Ok LGTM I'm merging this into master, 1.2 and 1.1

@andrewor14
Copy link
Contributor

Hey @numbnut do you have a JIRA account?

asfgit pushed a commit that referenced this pull request Nov 10, 2014
For me the core tests failed because there are two locale dependent parts in the code.
Look at the Jira ticket for details.

Why is it necessary to check the exception message in isBindCollision in
https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/util/Utils.scala#L1686
?

Author: Niklas Wilcke <[email protected]>

Closes #3036 from numbnut/core-test-fix and squashes the following commits:

1fb0d04 [Niklas Wilcke] Fixing locale dependend code and tests

(cherry picked from commit ed8bf1e)
Signed-off-by: Andrew Or <[email protected]>
asfgit pushed a commit that referenced this pull request Nov 10, 2014
For me the core tests failed because there are two locale dependent parts in the code.
Look at the Jira ticket for details.

Why is it necessary to check the exception message in isBindCollision in
https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/util/Utils.scala#L1686
?

Author: Niklas Wilcke <[email protected]>

Closes #3036 from numbnut/core-test-fix and squashes the following commits:

1fb0d04 [Niklas Wilcke] Fixing locale dependend code and tests

(cherry picked from commit ed8bf1e)
Signed-off-by: Andrew Or <[email protected]>
@asfgit asfgit closed this in ed8bf1e Nov 10, 2014
@numbnut
Copy link
Author

numbnut commented Nov 11, 2014

Yes I have a JIRA account. My name at JIRA is also numbnut. Why are you asking?
Went anything wrong?

@srowen
Copy link
Member

srowen commented Nov 11, 2014

@numbnut (Just so you can get the credit in JIRA.)

@numbnut
Copy link
Author

numbnut commented Nov 11, 2014

Ahh ok. Thank you!

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.

6 participants