You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by shijinkui <gi...@git.apache.org> on 2014/10/03 17:14:03 UTC
[GitHub] spark pull request: [SPARK-3781] code Style format
GitHub user shijinkui opened a pull request:
https://github.com/apache/spark/pull/2644
[SPARK-3781] code Style format
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shijinkui/spark styleFormat
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/2644.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 #2644
----
commit e00f13a554fb4a95996250e6cd65403fcc20b6e2
Author: shijinkui <sh...@163.com>
Date: 2014-09-29T05:34:02Z
code style format
commit 271e2a4a60ec2412f32966b65448c8ca25dc377f
Author: shijinkui <sh...@163.com>
Date: 2014-10-03T14:21:44Z
code format
commit 033af6a1bd2011b847633934ba719716d459aca8
Author: shijinkui <sh...@163.com>
Date: 2014-10-03T15:03:24Z
code style format
----
---
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-3781] code Style format
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18544762
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala ---
@@ -1019,7 +1020,7 @@ class DAGScheduler(
changeEpoch = true)
}
clearCacheLocs()
- if (stage.outputLocs.exists(_ == Nil)) {
+ if (stage.outputLocs.contains(Nil)) {
--- End diff --
I think this is equivalent but I'm not 100% sure.
---
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-3781] code Style format
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18565055
--- Diff: core/src/main/scala/org/apache/spark/ui/UIWorkloadGenerator.scala ---
@@ -17,11 +17,11 @@
package org.apache.spark.ui
-import scala.util.Random
-
-import org.apache.spark.{SparkConf, SparkContext}
import org.apache.spark.SparkContext._
import org.apache.spark.scheduler.SchedulingMode
+import org.apache.spark.{SparkConf, SparkContext}
+
+import scala.util.Random
--- End diff --
Spark code base convention is to import `scala` first. I have actually never seen a project that uses lexicographical sort order globally, but overall groups imports logically and then sorts. In any event, it's better to follow standard convention and put `scala` first.
---
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-3781] code Style format
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18561868
--- Diff: core/src/main/scala/org/apache/spark/broadcast/BroadcastManager.scala ---
@@ -59,7 +59,7 @@ private[spark] class BroadcastManager(
private val nextBroadcastId = new AtomicLong(0)
def newBroadcast[T: ClassTag](value_ : T, isLocal: Boolean) = {
- broadcastFactory.newBroadcast[T](value_, isLocal, nextBroadcastId.getAndIncrement())
+ broadcastFactory.newBroadcast[T](value_, isLocal, nextBroadcastId.getAndIncrement)
--- End diff --
val i: AtomicLong = new AtomicLong(0)
i.incrementAndGet()
Code:
0: new #16 // class java/util/concurrent/atomic/AtomicLong
3: dup
4: lconst_0
5: invokespecial #19 // Method java/util/concurrent/atomic/AtomicLong."<init>":(J)V
8: astore_2
9: aload_2
10: invokevirtual #23 // Method java/util/concurrent/atomic/AtomicLong.incrementAndGet:()J
13: pop2
14: return
val i: AtomicLong = new AtomicLong(0)
i.incrementAndGet
Code:
0: new #16 // class java/util/concurrent/atomic/AtomicLong
3: dup
4: lconst_0
5: invokespecial #19 // Method java/util/concurrent/atomic/AtomicLong."<init>":(J)V
8: astore_2
9: aload_2
10: invokevirtual #23 // Method java/util/concurrent/atomic/AtomicLong.incrementAndGet:()J
13: pop2
14: return
they are same byte code.
scala recommend having no () if having no param
---
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-3781] code Style format
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/2644#issuecomment-58251994
Hey @shijinkui I agree with most of the changes here. It seems that there are a few lines that exceed the 100 character limit however.
---
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-3781] code Style format
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18596253
--- Diff: core/src/main/scala/org/apache/spark/Aggregator.scala ---
@@ -40,10 +40,9 @@ case class Aggregator[K, V, C] (
def combineValuesByKey(iter: Iterator[_ <: Product2[K, V]]): Iterator[(K, C)] =
combineValuesByKey(iter, null)
- def combineValuesByKey(iter: Iterator[_ <: Product2[K, V]],
- context: TaskContext): Iterator[(K, C)] = {
+ def combineValuesByKey(iter: Iterator[_ <: Product2[K, V]], context: TaskContext): Iterator[(K, C)] = {
--- End diff --
Normally yes, but in Spark we also have a 100 character limit per line, so if we don't break it down into multiple lines it won't pass the style 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-3781] code Style format
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18544674
--- Diff: core/src/main/scala/org/apache/spark/broadcast/BroadcastManager.scala ---
@@ -59,7 +59,7 @@ private[spark] class BroadcastManager(
private val nextBroadcastId = new AtomicLong(0)
def newBroadcast[T: ClassTag](value_ : T, isLocal: Boolean) = {
- broadcastFactory.newBroadcast[T](value_, isLocal, nextBroadcastId.getAndIncrement())
+ broadcastFactory.newBroadcast[T](value_, isLocal, nextBroadcastId.getAndIncrement)
--- End diff --
I think the `()` should be there because this is not just a getter (it also increments)
---
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-3781] code Style format
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18544949
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -845,7 +845,7 @@ private[spark] object Utils extends Logging {
stdoutThread.join() // Wait for it to finish reading output
if (exitCode != 0) {
logError(s"Process $command exited with code $exitCode: $output")
- throw new SparkException(s"Process $command exited with code $exitCode")
--- End diff --
I think we should just keep this. We use string interpolation at many places.
---
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-3781] code Style format
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18562022
--- Diff: core/src/main/scala/org/apache/spark/scheduler/SchedulerBackend.scala ---
@@ -38,5 +42,4 @@ private[spark] trait SchedulerBackend {
* @return The application ID, or None if the backend does not provide an ID.
*/
def applicationId(): Option[String] = None
-
-}
+}
--- End diff --
OK
---
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-3781] code Style format
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2644#issuecomment-57810182
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: [SPARK-3781] code Style format
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/2644#issuecomment-58390539
Hey @shijinkui I think we're fairly confused as to which one is the latest PR. Is there a reason why you keep opening and closing PRs? If you make a change to the code you can always just push it to the same branch and it will show up in the same PR.
---
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-3781] code Style format
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18544867
--- Diff: core/src/main/scala/org/apache/spark/ui/UIWorkloadGenerator.scala ---
@@ -17,11 +17,11 @@
package org.apache.spark.ui
-import scala.util.Random
-
-import org.apache.spark.{SparkConf, SparkContext}
import org.apache.spark.SparkContext._
import org.apache.spark.scheduler.SchedulingMode
+import org.apache.spark.{SparkConf, SparkContext}
+
+import scala.util.Random
--- End diff --
Shouldn't scala be above Spark imports? I was under the impression that the order is java > scala > 3rd party > Spark.
---
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-3781] code Style format
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18565013
--- Diff: core/src/main/scala/org/apache/spark/broadcast/BroadcastManager.scala ---
@@ -59,7 +59,7 @@ private[spark] class BroadcastManager(
private val nextBroadcastId = new AtomicLong(0)
def newBroadcast[T: ClassTag](value_ : T, isLocal: Boolean) = {
- broadcastFactory.newBroadcast[T](value_, isLocal, nextBroadcastId.getAndIncrement())
+ broadcastFactory.newBroadcast[T](value_, isLocal, nextBroadcastId.getAndIncrement)
--- End diff --
No, the style convention is to omit () when the method has no side effects. `incrementAndGet` clearly has a side-effect, so should have ()
---
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-3781] code Style format
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18544810
--- Diff: core/src/main/scala/org/apache/spark/scheduler/SchedulerBackend.scala ---
@@ -38,5 +42,4 @@ private[spark] trait SchedulerBackend {
* @return The application ID, or None if the backend does not provide an ID.
*/
def applicationId(): Option[String] = None
-
-}
+}
--- End diff --
need new line
---
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-3781] code Style format
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18560510
--- Diff: core/src/main/scala/org/apache/spark/Aggregator.scala ---
@@ -40,10 +40,9 @@ case class Aggregator[K, V, C] (
def combineValuesByKey(iter: Iterator[_ <: Product2[K, V]]): Iterator[(K, C)] =
combineValuesByKey(iter, null)
- def combineValuesByKey(iter: Iterator[_ <: Product2[K, V]],
- context: TaskContext): Iterator[(K, C)] = {
+ def combineValuesByKey(iter: Iterator[_ <: Product2[K, V]], context: TaskContext): Iterator[(K, C)] = {
--- End diff --
i think method parameter should be in one line possibly, there is no necessary each param one line
:)
---
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-3781] code Style format
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18544914
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -265,15 +265,15 @@ private[spark] object Utils extends Logging {
/** Copy all data from an InputStream to an OutputStream */
def copyStream(in: InputStream,
- out: OutputStream,
- closeStreams: Boolean = false): Long =
+ out: OutputStream,
+ closeStreams: Boolean = false): Long =
--- End diff --
correct style should be
```
def copyStream(
in: InputStream,
out: OutputStream,
closeStreams: Boolean = false): Long = {
...
}
```
---
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-3781] code Style format
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on the pull request:
https://github.com/apache/spark/pull/2644#issuecomment-58302944
repush at #2704
---
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-3781] code Style format
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18562132
--- Diff: core/src/main/scala/org/apache/spark/ui/UIWorkloadGenerator.scala ---
@@ -17,11 +17,11 @@
package org.apache.spark.ui
-import scala.util.Random
-
-import org.apache.spark.{SparkConf, SparkContext}
import org.apache.spark.SparkContext._
import org.apache.spark.scheduler.SchedulingMode
+import org.apache.spark.{SparkConf, SparkContext}
+
+import scala.util.Random
--- End diff --
http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s3.3-import-statements
in ASCII sort order
---
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-3781] code Style format
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18544573
--- Diff: core/src/main/scala/org/apache/spark/Aggregator.scala ---
@@ -40,10 +40,9 @@ case class Aggregator[K, V, C] (
def combineValuesByKey(iter: Iterator[_ <: Product2[K, V]]): Iterator[(K, C)] =
combineValuesByKey(iter, null)
- def combineValuesByKey(iter: Iterator[_ <: Product2[K, V]],
- context: TaskContext): Iterator[(K, C)] = {
+ def combineValuesByKey(iter: Iterator[_ <: Product2[K, V]], context: TaskContext): Iterator[(K, C)] = {
--- End diff --
This is > 100 chars. The correct style here is
```
def combineValuesByKey(
iter: Iterator[...],
context: TaskContext): Iterator[...] = {
...
}
```
---
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-3781] code Style format
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui commented on a diff in the pull request:
https://github.com/apache/spark/pull/2644#discussion_r18562140
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -265,15 +265,15 @@ private[spark] object Utils extends Logging {
/** Copy all data from an InputStream to an OutputStream */
def copyStream(in: InputStream,
- out: OutputStream,
- closeStreams: Boolean = false): Long =
+ out: OutputStream,
+ closeStreams: Boolean = false): Long =
--- End diff --
yes
---
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-3781] code Style format
Posted by shijinkui <gi...@git.apache.org>.
Github user shijinkui closed the pull request at:
https://github.com/apache/spark/pull/2644
---
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