You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by numbnut <gi...@git.apache.org> on 2014/10/31 10:53:24 UTC
[GitHub] spark pull request: [Core] Locale dependent code
GitHub user numbnut opened a pull request:
https://github.com/apache/spark/pull/3036
[Core] Locale dependent code
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
core/src/main/scala/org/apache/spark/util/Utils.scala
?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/numbnut/spark core-test-fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/3036.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3036
----
commit 1fb0d04bfa1e4f8e0171394ceee85e25b6e073b7
Author: Niklas Wilcke <1w...@informatik.uni-hamburg.de>
Date: 2014-10-31T09:38:54Z
Fixing locale dependend code and tests
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62241212
Jenkins, this is ok to test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62241384
[Test build #23085 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/23085/consoleFull) for PR 3036 at commit [`1fb0d04`](https://github.com/apache/spark/commit/1fb0d04bfa1e4f8e0171394ceee85e25b6e073b7).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by numbnut <gi...@git.apache.org>.
Github user numbnut commented on a diff in the pull request:
https://github.com/apache/spark/pull/3036#discussion_r19661827
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -103,14 +105,16 @@ class UtilsSuite extends FunSuite {
val hour = minute * 60
def str = Utils.msDurationToString(_)
+ val sep = new DecimalFormatSymbols(Locale.getDefault()).getDecimalSeparator()
--- End diff --
I changed the title. Thanks for the hint.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62440840
Ok LGTM I'm merging this into master, 1.2 and 1.1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62439900
This doesn't just affect unit tests, which is why it's important: http://issues.apache.org/jira/browse/SPARK-4316
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62244045
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62439751
/cc @andrewor14 Any thoughts here? Some other users have started hitting this, too, so I think this is a high priority patch to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [Core] Locale dependent code
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3036#discussion_r19659592
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -103,14 +105,16 @@ class UtilsSuite extends FunSuite {
val hour = minute * 60
def str = Utils.msDurationToString(_)
+ val sep = new DecimalFormatSymbols(Locale.getDefault()).getDecimalSeparator()
--- End diff --
I think that's a good fix; just hard-coding `Locale.ENGLISH` is also a fix. On the one hand that's less friendly to the non-English-speaking world, but might avoid some unexpected complications of this form. I expect they will be cosmetic only. I'm indifferent; wonder if anyone else has a though.
BTW you should put the SPARK-XXXX in the title and maybe a more action-oriented title like "Accommodate non-English Locales in unit tests"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/3036
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3036#discussion_r20048772
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -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) {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by numbnut <gi...@git.apache.org>.
Github user numbnut commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62524351
Yes I have a JIRA account. My name at JIRA is also numbnut. Why are you asking?
Went anything wrong?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [Core] Locale dependent code
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-61239351
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [Core] Locale dependent code
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3036#discussion_r19659505
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -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) {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62244044
[Test build #23085 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/23085/consoleFull) for PR 3036 at commit [`1fb0d04`](https://github.com/apache/spark/commit/1fb0d04bfa1e4f8e0171394ceee85e25b6e073b7).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62441291
Hey @numbnut do you have a JIRA account?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62440690
add to whitelist
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 `BindException`s 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by numbnut <gi...@git.apache.org>.
Github user numbnut commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62525026
Ahh ok. Thank you!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-4169] [Core] Accommodate non-English Lo...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/3036#issuecomment-62524790
@numbnut (Just so you can get the credit in JIRA.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org