You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by jaceklaskowski <gi...@git.apache.org> on 2014/05/13 01:28:25 UTC
[GitHub] spark pull request: String interpolation + some other small change...
GitHub user jaceklaskowski opened a pull request:
https://github.com/apache/spark/pull/748
String interpolation + some other small changes
After having been invited to make the change in https://github.com/apache/spark/commit/6bee01dd04ef73c6b829110ebcdd622d521ea8ff#commitcomment-6284165 by @witgo.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jaceklaskowski/spark sparkenv-string-interpolation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/748.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 #748
----
commit be6ebaca1cfabcdbbf7bb7f84e1bf72b8c3330dc
Author: Jacek Laskowski <ja...@japila.pl>
Date: 2014-05-12T23:26:08Z
String interpolation + some other small changes
----
---
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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski commented on a diff in the pull request:
https://github.com/apache/spark/pull/748#discussion_r12665202
--- Diff: core/src/main/scala/org/apache/spark/SparkEnv.scala ---
@@ -296,18 +297,15 @@ object SparkEnv extends Logging {
// System properties that are not java classpaths
val systemProperties = System.getProperties.iterator.toSeq
- val otherProperties = systemProperties.filter { case (k, v) =>
+ val otherProperties = systemProperties.filter { case (k, _) =>
k != "java.class.path" && !k.startsWith("spark.")
}.sorted
// Class paths including all added jars and files
- val classPathProperty = systemProperties.find { case (k, v) =>
- k == "java.class.path"
- }.getOrElse(("", ""))
- val classPathEntries = classPathProperty._2
+ val classPathEntries = javaClassPath
.split(File.pathSeparator)
--- End diff --
Yes, I did, but not within Spark, but outside, in REPL. As a matter of fact, [Properties.javaClassPath](https://github.com/scala/scala/blob/2.12.x/src/library/scala/util/Properties.scala#L126) ends up calling [System.getProperty(name, alt)](https://github.com/scala/scala/blob/2.12.x/src/library/scala/util/Properties.scala#L52) so the change is truly a refactoring (to ease reading the class later on).
---
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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/748#issuecomment-43147538
Thanks, looks good. I'll merge this into master.
---
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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/748#issuecomment-42905084
Jenkins, test this please. This looks good to me, pending tests (I'd also like to test this locally).
---
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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/748
---
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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/748#issuecomment-42901509
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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/748#issuecomment-42907884
Merged build finished. All automated tests 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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/748#discussion_r12666435
--- Diff: core/src/main/scala/org/apache/spark/SparkEnv.scala ---
@@ -296,18 +297,15 @@ object SparkEnv extends Logging {
// System properties that are not java classpaths
val systemProperties = System.getProperties.iterator.toSeq
- val otherProperties = systemProperties.filter { case (k, v) =>
+ val otherProperties = systemProperties.filter { case (k, _) =>
k != "java.class.path" && !k.startsWith("spark.")
}.sorted
// Class paths including all added jars and files
- val classPathProperty = systemProperties.find { case (k, v) =>
- k == "java.class.path"
- }.getOrElse(("", ""))
- val classPathEntries = classPathProperty._2
+ val classPathEntries = javaClassPath
.split(File.pathSeparator)
--- End diff --
Great, this looks good. Thanks for checking it out.
---
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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on a diff in the pull request:
https://github.com/apache/spark/pull/748#discussion_r12561839
--- Diff: core/src/main/scala/org/apache/spark/SparkEnv.scala ---
@@ -296,18 +297,15 @@ object SparkEnv extends Logging {
// System properties that are not java classpaths
val systemProperties = System.getProperties.iterator.toSeq
- val otherProperties = systemProperties.filter { case (k, v) =>
+ val otherProperties = systemProperties.filter { case (k, _) =>
k != "java.class.path" && !k.startsWith("spark.")
}.sorted
// Class paths including all added jars and files
- val classPathProperty = systemProperties.find { case (k, v) =>
- k == "java.class.path"
- }.getOrElse(("", ""))
- val classPathEntries = classPathProperty._2
+ val classPathEntries = javaClassPath
.split(File.pathSeparator)
--- End diff --
Just wondering - did you test this locally? It would be good to make sure that `javaClassPath` returns the exact same format (e.g. including the line termination character) as what we were doing before.
---
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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/748#issuecomment-42905203
Merged build started.
---
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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/748#issuecomment-42907890
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14917/
---
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.
---
[GitHub] spark pull request: String interpolation + some other small change...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/748#issuecomment-42905196
Merged build triggered.
---
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.
---