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 2016/05/26 17:47:58 UTC
[GitHub] spark pull request: [CORE][SQL][MINOR] Scaladoc fixes + string int...
GitHub user jaceklaskowski opened a pull request:
https://github.com/apache/spark/pull/13329
[CORE][SQL][MINOR] Scaladoc fixes + string interpolation
## What changes were proposed in this pull request?
Scaladoc fixes + string interpolation for logging
## How was this patch tested?
local build + manual testing
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jaceklaskowski/spark minor-fixes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/13329.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 #13329
----
commit 2e794dcbf69af5e6d399e3d07cec002bbd573bc9
Author: Jacek Laskowski <ja...@japila.pl>
Date: 2016-05-26T17:45:26Z
[CORE][SQL][MINOR] Scaladoc fixes + string interpolation
----
---
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][SQL][MINOR] Scaladoc fixes + string int...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-222140528
The issue with `nonEmpty` is that you could easily miss the negation (and that's why Scala offers `nonEmpty`). I don't think it's the final solution, but certainly believe it's far better for future readers to know what's going on in these lines. I believe the changes improve readability (but can happily revert _some_ if you point me at the places that need this).
It's just me to believe that by doing these small changes the code becomes more readable. I spent enough time with it to think it needs so (more often than I'm proposing).
Please guide me to learn your coding style. Thanks!
---
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][SQL][MINOR] Scaladoc fixes + string int...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/13329#discussion_r64859729
--- Diff: yarn/src/main/scala/org/apache/spark/scheduler/cluster/YarnScheduler.scala ---
@@ -31,9 +31,8 @@ private[spark] class YarnScheduler(sc: SparkContext) extends TaskSchedulerImpl(s
Logger.getLogger(classOf[RackResolver]).setLevel(Level.WARN)
}
- // By default, rack is unknown
override def getRackForHost(hostPort: String): Option[String] = {
- val host = Utils.parseHostPort(hostPort)._1
+ val (host, _) = Utils.parseHostPort(hostPort)
--- End diff --
is this even an improvement?
---
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][SQL][MINOR] Scaladoc fixes + string int...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-222270008
Thanks @rxin and @srowen for your help and patience! I'll close the pull request.
---
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][SQL][MINOR] Scaladoc fixes + string int...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-221945121
**[Test build #59395 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59395/consoleFull)** for PR 13329 at commit [`2e794dc`](https://github.com/apache/spark/commit/2e794dcbf69af5e6d399e3d07cec002bbd573bc9).
---
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][SQL][MINOR] Scaladoc fixes + string int...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-221974375
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/59395/
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: [CORE][SQL][MINOR] Scaladoc fixes + string int...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski commented on a diff in the pull request:
https://github.com/apache/spark/pull/13329#discussion_r64900321
--- Diff: yarn/src/main/scala/org/apache/spark/scheduler/cluster/YarnScheduler.scala ---
@@ -31,9 +31,8 @@ private[spark] class YarnScheduler(sc: SparkContext) extends TaskSchedulerImpl(s
Logger.getLogger(classOf[RackResolver]).setLevel(Level.WARN)
}
- // By default, rack is unknown
override def getRackForHost(hostPort: String): Option[String] = {
- val host = Utils.parseHostPort(hostPort)._1
+ val (host, _) = Utils.parseHostPort(hostPort)
--- End diff --
It is when you agree that you could easily (?) miss `_1` at the very end. I do agree and I did miss it few times while reviewing that piece of code. Opinions may vary and I can happily revert this change if requested (I need your advice to learn your coding style).
---
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][SQL][MINOR] Scaladoc fixes + string int...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-222245541
In this cases it seems like the value is pretty small. I have another pr that does some api annotation cleanups. I will port your typo fixes over 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE][SQL][MINOR] Scaladoc fixes + string int...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-222242718
How am I supposed to read this? Do you want me to...forget about the changes? All of them or just some? Which one would you accept since @srowen said: "Some of this is OK"? It _appears_ there's _some_ merit in the change.
---
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][SQL][MINOR] Scaladoc fixes + string int...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-221974373
Merged build finished. 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: [CORE][SQL][MINOR] Scaladoc fixes + string int...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-222228694
I'm with @srowen on this one. Not sure about the "improvements" here are indeed improvements.
---
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][SQL][MINOR] Scaladoc fixes + string int...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski closed the pull request at:
https://github.com/apache/spark/pull/13329
---
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][SQL][MINOR] Scaladoc fixes + string int...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-221979133
Some of this is OK, some of it isn't an improvement. I don't think we should just change a few instances of format to interpolation, nor do I think it is necessarily worth it to change everything all at once. Same with stuff like `nonEmpty`. This is just too hodge-podge to bother with.
---
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][SQL][MINOR] Scaladoc fixes + string int...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-221974106
**[Test build #59395 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59395/consoleFull)** for PR 13329 at commit [`2e794dc`](https://github.com/apache/spark/commit/2e794dcbf69af5e6d399e3d07cec002bbd573bc9).
* 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: [CORE][SQL][MINOR] Scaladoc fixes + string int...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/13329#issuecomment-222256093
In roughly descending order, here are possible positive results of a change:
- Fix a bug
- Improve performance
- Improve tests
- Clarify user-visible documentation (log/exceptions, javadoc)
- Improve code style or consistency
- Improve code comments or other non-functional clarification
and here are some possible costs or risks:
- introduces a bug
- introduces a performance regression
- adds dependencies
- adds to maintenance overhead
- decreases code consistency
- introduces non-trivial merge conflict problems
- takes time to review and merge
It's not that there aren't positive changes here, in isolation, but they're just marginal. Although interpolation is probably better than format(), it's inconsistent both before and after this change. Same with nonEmpty. Some of these I don't think are improvements, like "fetch" -> "fetches".
But why wouldn't you want to merge any helpful change? It's really the cost of reviewing it, and distantly, the merge conflict issue. I don't mind people getting started by contributing small changes, and paying that cost in order to on-board people into a process and community. I also think the occasional tiny change is fine.
I should also say that I do find it reasonable to fix up code in these small details when already changing it for other purposes.
What I don't want to do is fix 1 issue piecemeal over 10 changes. Either we shouldn't really bother, or should fix it one go as much as possible. For example, although we should probably talk about it, I'd be more in favor of changing all `!...isEmpty` to `nonEmpty` in one go than this change.
I also prefer to see experienced contributors "graduate" towards more significant changes or reviewing/shepherding other changes.
I have heard from a few sources that some organizations may incentivize employees to contribute to Spark (good) and measure by number of JIRAs or pull requests (bad). I don't think that's what's happening here, but another reason I personally want to push back on _everyone_ submitting bitty 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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org