You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by witgo <gi...@git.apache.org> on 2014/04/27 04:45:20 UTC
[GitHub] spark pull request: Fix SPARK-1629: Spark should inline use of com...
GitHub user witgo opened a pull request:
https://github.com/apache/spark/pull/569
Fix SPARK-1629: Spark should inline use of commons-lang `SystemUtils.IS_...
...OS_WINDOWS`
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/witgo/spark SPARK-1629
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/569.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 #569
----
commit 49e248e7a055c50586bf1f4170c5404566adba23
Author: witgo <wi...@qq.com>
Date: 2014-04-27T02:44:29Z
Fix SPARK-1629: Spark should inline use of commons-lang `SystemUtils.IS_OS_WINDOWS`
----
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by witgo <gi...@git.apache.org>.
Github user witgo commented on a diff in the pull request:
https://github.com/apache/spark/pull/569#discussion_r12027354
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1056,4 +1055,11 @@ private[spark] object Utils extends Logging {
def getHadoopFileSystem(path: String): FileSystem = {
getHadoopFileSystem(new URI(path))
}
+
+ /**
+ * return true if this is Windows.
+ */
+ def isWindows = Option(System.getProperty("os.name")).
--- End diff --
@srowen
```scala
def isWindows(): Boolean = {
try {
val osName = System.getProperty("os.name")
osName != null && osName.startsWith("Windows")
} catch {
case e: SecurityException => (log a warning and return false)
}
}
````
You think here will be thrown a SecurityException .Why?
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-41524423
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.
---
Re: [GitHub] spark pull request: Fix SPARK-1629: Spark should inline
use of com...
Posted by Sean Owen <so...@cloudera.com>.
Agree, this has not changed since java 1.1 or so. The niggling remaining
issue is that a test does use it too so technically needs the dependency
tweak anyway. And tachyon already brings it in.
That use can't be inlined but pretty certain is just a string replace. I
don't know if tachyon's use is likewise trivial.
On May 5, 2014 12:12 AM, "pwendell" <gi...@git.apache.org> wrote:
> Github user pwendell commented on the pull request:
>
> https://github.com/apache/spark/pull/569#issuecomment-42150290
>
> I'm ambivalent between inlining this and fixing up the dependency. In
> general we should probably rely on other utilities where possible, but if
> the JVM ever changes the way windows machines identify themselves in a
> non-backwards-compatible fashion, I will eat my shoe.
>
>
> ---
> 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: Fix SPARK-1629: Spark should inline use of com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-42150290
I'm ambivalent between inlining this and fixing up the dependency. In general we should probably rely on other utilities where possible, but if the JVM ever changes the way windows machines identify themselves in a non-backwards-compatible fashion, I will eat my shoe.
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-41526093
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: Fix SPARK-1629: Spark should inline use of com...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-42132244
If we are depending on something and bundling it, we might as well use it
instead of duplicating code and having to maintain the changes : assuming
it is intutive to use.
Since apache commons is used by whole bunch of projects, bugs would be
better reported and fixed there than hacked up code within spark.
The long term maintenance cost adds up for these 'small' changes.
Things like os.name.tolowercase startswith windows or file.separator ==\\
are hacks we should avoid and let others handle if being maintained.
Regards
Mridul
On May 4, 2014 12:55 PM, "Sean Owen" <no...@github.com> wrote:
> Yes, but those are transitive dependencies. core happens to bring in both
> lang and lang3, and this was using the old lang dependency even. It would
> be correct-er to either depend directly on it, or not use it. To me it
> seems better to inline what amounts to one-liner access of a standard
> system property.
>
> I think it's also true that Guava is the go-to utility package and would
> be better to use Guava where possible rather than encourage use of lang too.
>
> The use of lang3 in ReplSuite should also either be 'inlined', or else
> supplemented with a direct test-scope dependency. It's just a test, so not
> as big of an issue. I think the one use can even be replaced with a simple
> escape, which I presume is there to handle backslash on Windows.
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/apache/spark/pull/569#issuecomment-42126473>
> .
>
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/569
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-42137398
That makes sense. Since Tachyon now brings in `lang3`, it is no extra burden to use it. It would rationalize the use in `ReplSuite`. Anyone second a PR a to bring _back_ use of Commons Lang, but the latest `lang3` version?
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-41486930
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: Fix SPARK-1629: Spark should inline use of com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-42116492
The problem was that the project was not depending on commons lang already.
The code is the same as is contained in commons lang. The property being
referenced is a standard JVM property.
On May 3, 2014 9:34 PM, "Mridul Muralidharan" <no...@github.com>
wrote:
> I am still catching up on PR's and bugs.
> Why was this changed ?
> Hacky solutions based on string parsing of properties lead to fragility in
> case of changes in future.
> Since we are already depending on apache commons, makes sense to delegate
> to that.
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/apache/spark/pull/569#issuecomment-42115677>
> .
>
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/569#discussion_r12027664
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1058,6 +1057,12 @@ private[spark] object Utils extends Logging {
}
/**
+ * return true if this is Windows.
+ */
+ def isWindows = Option(System.getProperty("os.name")).
--- End diff --
Regarding `SecurityException`, `System.getProperty` will often be disallowed in an environment where the security manager has been enabled. I strongly suspect that the rest of Spark fails already in such an environment for other reasons, and you could argue that the only reasonable response here is to fail rather than "guess" that the environment is not Windows. But I put it in mostly to mimic `SystemUtils`. It could go either way.
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-41524258
Jenkins, test this please.
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-41526096
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14526/
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-42126473
Yes, but those are transitive dependencies. `core` happens to bring in both lang and lang3, and this was using the old lang dependency even. It would be correct-er to either depend directly on it, or not use it. To me it seems better to inline what amounts to one-liner access of a standard system property.
I think it's also true that Guava is the go-to utility package and would be better to use Guava where possible rather than encourage use of lang too.
The use of lang3 in ReplSuite should also either be 'inlined', or else supplemented with a direct test-scope dependency. It's just a test, so not as big of an issue. I think the one use can even be replaced with a simple escape, which I presume is there to handle backslash on Windows.
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-42120375
Maven dependency still shows org.apache.commons:commons-lang3:jar - am I missing something here ?
Btw, we do depend on it for repl tests too ...
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-41524433
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: Fix SPARK-1629: Spark should inline use of com...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-42144008
Yes, lang3 - not lang : some of the methods (for example, the escape method
used in repl) is actually broken in lang, but works in lang3.
On Sun, May 4, 2014 at 10:09 PM, Sean Owen <no...@github.com> wrote:
> That makes sense. Since Tachyon now brings in lang3, it is no extra
> burden to use it. It would rationalize the use in ReplSuite. Anyone
> second a PR a to bring *back* use of Commons Lang, but the latest lang3version?
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/apache/spark/pull/569#issuecomment-42137398>
> .
>
---
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: Fix SPARK-1629: Spark should inline use of com...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-41522557
Jenkins, test this please.
Looks good to me, pending 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.
---
[GitHub] spark pull request: Fix SPARK-1629: Spark should inline use of com...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/569#issuecomment-42115677
I am still catching up on PR's and bugs.
Why was this changed ?
Hacky solutions based on string parsing of properties lead to fragility in case of changes in future.
Since we are already depending on apache commons, makes sense to delegate to that.
---
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.
---