[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user saucam closed the pull request at: https://github.com/apache/spark/pull/8678 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user saucam commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139459104 @andrewor14 , @srowen thanks for the feedback. I was actually working on a very low latency spark job (300 - 500 ms) , and thought it better to improve obvious things. It might not be applicable for wider audience, as suggested, closing this 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user srowen commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139340111 OK I support closing it too. It's negligible. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user andrewor14 commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139339394 I just think having a match evaluated in a method parameter makes the code look more complicated. Elsewhere in Spark we try to simplify the existing code in the opposite direction, so I see this as some sort of regression. If it's actually a significant source of performance improvement then I would be OK with sacrificing readability, but that's not the case here. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user srowen commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139337504 @andrewor14 I agree with your comment on the first revision, but just inlining the string argument as the latest commit does seems to simplify the code a tiny bit, and maybe gain a negligible bit of speed. Are you looking at the result after the second commit? i'm also fine closing this as it's pretty trivial, but I don't mind the change in its second form. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user andrewor14 commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139336887 No, it's never OK to sacrifice readability for a performance improvement that doesn't actually matter. Even changing it to a def adds complexity because now you have nested methods. Thanks for your work, but please close this 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139333857 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139333858 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42271/ 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139333517 [Test build #42271 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42271/console) for PR 8678 at commit [`42bf3ef`](https://github.com/apache/spark/commit/42bf3efa7baf01ae1649fd43f222553cbd4c2a49). * 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user saucam commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139331711 @andrewor14 for the "harder to read part" initially i had changed the val to a def ; the rest was the same , I am sure it was not affecting the readability if thats your concern here, but @srowen suggested to pass the value directly (don;t know why) I can revert it to my original change if you prefer that way. "The reduce in delay is extremely negligible" agreed! , but still it is there ! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user andrewor14 commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139330101 I don't think this is worth doing. It makes the code harder to read, and the string computation is not at all expensive. This isn't even a hot code path, so the reduce in scheduling delay here is extremely negligible. I would prefer to close this issue. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139284091 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139286022 [Test build #42271 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42271/consoleFull) for PR 8678 at commit [`42bf3ef`](https://github.com/apache/spark/commit/42bf3efa7baf01ae1649fd43f222553cbd4c2a49). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139284041 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user saucam commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139283491 @srowen thanks for the detailed explanation, passed the value directly --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/8678#discussion_r39155302
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala
---
@@ -901,7 +901,7 @@ class DAGScheduler(
// the stage as completed here in case there are no tasks to run
markStageAsFinished(stage, None)
- val debugString = stage match {
+ def debugString: String = stage match {
--- End diff --
I don't think so, because `logDebug` is declared as `logDebug(msg: =>
String)`; the laziness is built in. As in...
```
scala> def foo(eval: Boolean, msg: => String) = if (eval) println(msg)
foo: (eval: Boolean, msg: => String)Unit
scala> val i = new java.util.concurrent.atomic.AtomicInteger()
i: java.util.concurrent.atomic.AtomicInteger = 0
scala> foo(true, s"${i.incrementAndGet}")
1
scala> i
res5: java.util.concurrent.atomic.AtomicInteger = 1
scala> foo(false, s"${i.incrementAndGet}")
scala> i
res7: java.util.concurrent.atomic.AtomicInteger = 1
```
It wasn't evaluated in the second instance.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user saucam commented on a diff in the pull request:
https://github.com/apache/spark/pull/8678#discussion_r39153871
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala
---
@@ -901,7 +901,7 @@ class DAGScheduler(
// the stage as completed here in case there are no tasks to run
markStageAsFinished(stage, None)
- val debugString = stage match {
+ def debugString: String = stage match {
--- End diff --
Even if its not heavy , we can easily make it lazy.
By passing it directly , it will still always evaluate the expression 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/8678#discussion_r39151883
--- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala
---
@@ -901,7 +901,7 @@ class DAGScheduler(
// the stage as completed here in case there are no tasks to run
markStageAsFinished(stage, None)
- val debugString = stage match {
+ def debugString: String = stage match {
--- End diff --
Is this really expensive to compute? it doesn't look that heavy. But you
can already just pass the expression as the argument to `logDebug` and I think
it will be lazily evaluated? Only mostly sure about 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139119751 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42233/ 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139119750 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139119679 [Test build #42233 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42233/console) for PR 8678 at commit [`e4c4c10`](https://github.com/apache/spark/commit/e4c4c10db44cbec79e190265fc0351731ff664ec). * This patch **passes all tests**. * This patch merges cleanly. * This patch adds the following public classes _(experimental)_: * `class SparkHadoopWriter(jobConf: JobConf)` * `class BlockRDD[T: ClassTag](sc: SparkContext, @transient val blockIds: Array[BlockId])` * `class ZippedWithIndexRDD[T: ClassTag](prev: RDD[T]) extends RDD[(T, Long)](prev) ` * `class Checkpoint(ssc: StreamingContext, val checkpointTime: Time)` * `abstract class InputDStream[T: ClassTag] (ssc_ : StreamingContext)` * `abstract class ReceiverInputDStream[T: ClassTag](ssc_ : StreamingContext)` --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139100597 [Test build #42233 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42233/consoleFull) for PR 8678 at commit [`e4c4c10`](https://github.com/apache/spark/commit/e4c4c10db44cbec79e190265fc0351731ff664ec). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139100401 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [CORE][SPARK-10527]: Minor enhancement to eval...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/8678#issuecomment-139100390 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
