[GitHub] spark pull request: [SPARK-4772] Clear local copies of accumulator...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-67386266
  
I've merged this into `branch-1.2`, so this fix will be included in Spark 
1.2.1.


---
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: [SPARK-4772] Clear local copies of accumulator...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/3570


---
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: [SPARK-4772] Clear local copies of accumulator...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66415593
  
Thanks for updating the description.  This looks good to me, so I'm going 
to merge this into `master`, `branch-1.0`, and `branch-1.1` (and I'll tag it 
for a post-release backport into `branch-1.2`).  Thanks again for this fix!


---
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: [SPARK-4772] Clear local copies of accumulator...

[nkronenfeld]

Github user nkronenfeld commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66399588
  
sorry, must have accidentally hit cancel instead of comment the first time. 
 Should be set now.


---
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: [SPARK-4772] Clear local copies of accumulator...

[nkronenfeld]

Github user nkronenfeld commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66398560
  
I thought I'd done so, it looks like it lost my changes
I'll fix that asap



---
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: [SPARK-4772] Clear local copies of accumulator...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66398459
  
LGTM.  Just in case you missed my earlier comment, are you still planning 
to update the PR description to reflect the actual changes vs. the ones you had 
planned?


---
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: [SPARK-4772] Clear local copies of accumulator...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66391021
  
  [Test build #24274 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24274/consoleFull)
 for   PR 3570 at commit 
[`a581f3f`](https://github.com/apache/spark/commit/a581f3f99e072b39c3dcc0a103306b933ceea05b).
 * 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: [SPARK-4772] Clear local copies of accumulator...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66391029
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24274/
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: [SPARK-4772] Clear local copies of accumulator...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66383538
  
@kayousterhout I'm glad to see that changing it to `private[spark]` fixed 
things without requiring a MiMa exclude for the `private -> private[spark]` 
change.  This is going to make it _much_ easier to backport this into 
maintenance branches, since I won't have to move the MiMa exclude on each 
`cherry-pick`.


---
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: [SPARK-4772] Clear local copies of accumulator...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66383350
  
  [Test build #24274 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24274/consoleFull)
 for   PR 3570 at commit 
[`a581f3f`](https://github.com/apache/spark/commit/a581f3f99e072b39c3dcc0a103306b933ceea05b).
 * This patch merges cleanly.


---
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: [SPARK-4772] Clear local copies of accumulator...

[nkronenfeld]

Github user nkronenfeld commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66380241
  
comment fixed.  I'm trying to test the MiMa related changes to see if they 
work, and having problems running mima on my machine.  I'll probably just push 
them in the suggested form to see if they pass on Jenkins.  


---
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: [SPARK-4772] Clear local copies of accumulator...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/3570#discussion_r21573219
  
--- Diff: core/src/main/scala/org/apache/spark/Accumulators.scala ---
@@ -281,7 +282,9 @@ object AccumulatorParam {
 private object Accumulators {
   // TODO: Use soft references? => need to make readObject work properly 
then
   val originals = Map[Long, Accumulable[_, _]]()
-  val localAccums = Map[Thread, Map[Long, Accumulable[_, _]]]()
+  val localAccums = new ThreadLocal[Map[Long, Accumulable[_, _]]]() {
+override protected def initialValue() = Map[Long, Accumulable[_, _]]()
+  }
   var lastId: Long = 0
--- End diff --

Oh, I was just observing that this is only read through the `newIdI()` 
method and that it's effectively being used like an AtomicInteger.  Just 
another example of how this particular part of the code is kind of old / 
out-of-sync with the style of the rest of the codebase.  Don't worry about it; 
we can do a larger cleanup pass on this later.


---
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: [SPARK-4772] Clear local copies of accumulator...

[nkronenfeld]

Github user nkronenfeld commented on a diff in the pull request:

https://github.com/apache/spark/pull/3570#discussion_r21572834
  
--- Diff: core/src/main/scala/org/apache/spark/Accumulators.scala ---
@@ -281,7 +282,9 @@ object AccumulatorParam {
 private object Accumulators {
   // TODO: Use soft references? => need to make readObject work properly 
then
   val originals = Map[Long, Accumulable[_, _]]()
-  val localAccums = Map[Thread, Map[Long, Accumulable[_, _]]]()
+  val localAccums = new ThreadLocal[Map[Long, Accumulable[_, _]]]() {
+override protected def initialValue() = Map[Long, Accumulable[_, _]]()
+  }
   var lastId: Long = 0
--- End diff --

you mean the lastId?
That should only ever get used on the client - it's only called from the 
constructor of an individual accumulator, and if someone is creating one of 
those on a worker, they're already in trouble - so it shoudl be ok as is.


---
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: [SPARK-4772] Clear local copies of accumulator...

[kayousterhout]

Github user kayousterhout commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66360844
  
To fix the MiMA problem, can you instead make Accumulators a private[spark] 
object? No one I've asked seems to understand what "private" even means in this 
context -- private[spark] describes the desired semantics (based on my 
understanding), and doing that also removes the need for the MiMA exception (or 
at least it did for #3622)


---
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: [SPARK-4772] Clear local copies of accumulator...

[andrewor14]

Github user andrewor14 commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66357628
  
LGTM, nice catch.


---
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: [SPARK-4772] Clear local copies of accumulator...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66346390
  
This looks good to me.  If you don't mind, could you update the pull 
request description to more accurately describe the change that we're actually 
committing?  This is important because that description will become the actual 
commit message.

Also, it looks like the MiMa issue could have been caused by `private 
object` maybe having different semantics that we expected; we ran into a 
similar issue over at #3622.


---
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: [SPARK-4772] Clear local copies of accumulator...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/3570#discussion_r21557013
  
--- Diff: core/src/main/scala/org/apache/spark/Accumulators.scala ---
@@ -281,7 +282,9 @@ object AccumulatorParam {
 private object Accumulators {
   // TODO: Use soft references? => need to make readObject work properly 
then
   val originals = Map[Long, Accumulable[_, _]]()
-  val localAccums = Map[Thread, Map[Long, Accumulable[_, _]]]()
+  val localAccums = new ThreadLocal[Map[Long, Accumulable[_, _]]]() {
+override protected def initialValue() = Map[Long, Accumulable[_, _]]()
+  }
   var lastId: Long = 0
--- End diff --

Not related to your changes and I don't expect you to fix it, but this 
could be an `AtomicInteger` instead.


---
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: [SPARK-4772] Clear local copies of accumulator...

[nkronenfeld]

Github user nkronenfeld commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-66238332
  
Any word on this?


---
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: [SPARK-4772] Clear local copies of accumulator...

[nkronenfeld]

Github user nkronenfeld commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-65883848
  
oh, a note for when you're reviewing - I didn't move the clear call, I just 
added a second one; I saw no particular harm in leaving the old one there too, 
just in case, but I can't see it doing all that much anymore - it should always 
be a no-op now.  I'd be happier removing it if, again, I could figure out a 
good unit test to make sure all was functioning properly when I did so.  But I 
would be totally open to removing it in the interests of code cleanliness if 
you want.


---
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: [SPARK-4772] Clear local copies of accumulator...

[nkronenfeld]

Github user nkronenfeld commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-65883762
  
great... I think outside the mima issue, it should be all set, unless I can 
figure out a way to unit test it.  So far, my best methods of testing it 
involve instrumenting the code in ways I shouldn't check in.


---
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: [SPARK-4772] Clear local copies of accumulator...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-65883736
  
No, I'd leave it.  I just thought I'd mention it so that we eventually
investigate.  I'll finish reviewing this PR later this weekend.

On Fri, Dec 5, 2014 at 7:27 PM, Nathan Kronenfeld 
wrote:

> Should I back out the correction to the mima failure?
>
> —
> Reply to this email directly or view it on GitHub
> .
>


---
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: [SPARK-4772] Clear local copies of accumulator...

[nkronenfeld]

Github user nkronenfeld commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-65883687
  
Should I back out the correction to the mima failure? 


---
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: [SPARK-4772] Clear local copies of accumulator...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-65873447
  
The MiMa failure is surprising, since that class was marked as `private` 
and therefore shouldn't have been subject to compatibility checks.  
@ScrapCodes, do you know if this is a MiMa bug?  Do we have a JIRA for this?


---
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: [SPARK-4772] Clear local copies of accumulator...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-65870758
  
  [Test build #24195 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24195/consoleFull)
 for   PR 3570 at commit 
[`b6c2180`](https://github.com/apache/spark/commit/b6c2180f98e2fc35970b0c646c189b376c949255).
 * 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: [SPARK-4772] Clear local copies of accumulator...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-65870763
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24195/
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: [SPARK-4772] Clear local copies of accumulator...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/3570#issuecomment-65861501
  
  [Test build #24195 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24195/consoleFull)
 for   PR 3570 at commit 
[`b6c2180`](https://github.com/apache/spark/commit/b6c2180f98e2fc35970b0c646c189b376c949255).
 * This patch merges cleanly.


---
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