You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by tedyu <gi...@git.apache.org> on 2016/04/29 10:48:12 UTC
[GitHub] spark pull request: Use ConcurrentHashMap in place of HashMap for ...
GitHub user tedyu opened a pull request:
https://github.com/apache/spark/pull/12776
Use ConcurrentHashMap in place of HashMap for NewAccumulator.originals
## What changes were proposed in this pull request?
This PR proposes to use ConcurrentHashMap in place of HashMap for NewAccumulator.originals
## How was this patch tested?
Existing unit test suite
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tedyu/spark master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/12776.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 #12776
----
commit f8eb88ea8ba11df955d05a52e5cf1f6037512842
Author: tedyu <yu...@gmail.com>
Date: 2016-04-29T08:46:56Z
Use ConcurrentHashMap in place of HashMap for NewAccumulator.originals
----
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215737724
Merged build finished. Test FAILed.
---
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-15003] Use ConcurrentHashMap in place o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215782655
**[Test build #57340 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57340/consoleFull)** for PR 12776 at commit [`cffbc13`](https://github.com/apache/spark/commit/cffbc13af3a03e0df6bdc3ea3763dd66221b839a).
* This patch **fails Spark unit 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: Use ConcurrentHashMap in place of HashMap for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215737730
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57339/
Test FAILed.
---
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-15003] Use ConcurrentHashMap in place o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215796708
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57342/
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215735915
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57338/
Test FAILed.
---
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-15003] Use ConcurrentHashMap in place o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215796701
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61551567
--- Diff: core/src/main/scala/org/apache/spark/NewAccumulator.scala ---
@@ -199,7 +199,8 @@ private[spark] object AccumulatorContext {
* TODO: Don't use a global map; these should be tied to a SparkContext (SPARK-13051).
*/
@GuardedBy("AccumulatorContext")
- private val originals = new java.util.HashMap[Long, jl.ref.WeakReference[NewAccumulator[_, _]]]
+ private val originals =
+ new java.util.concurrent.ConcurrentHashMap[Long, jl.ref.WeakReference[NewAccumulator[_, _]]]
--- End diff --
Import this class?
---
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-15003] Use ConcurrentHashMap in place o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215852754
**[Test build #57347 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57347/consoleFull)** for PR 12776 at commit [`ff8ed2c`](https://github.com/apache/spark/commit/ff8ed2c7dc656d0f9a2d739ccddf3e907972118e).
* 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: [SPARK-15003] Use ConcurrentHashMap in place o...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61593377
--- Diff: core/src/test/scala/org/apache/spark/InternalAccumulatorSuite.scala ---
@@ -194,7 +194,8 @@ class InternalAccumulatorSuite extends SparkFunSuite with LocalSparkContext {
}
// Make sure the same set of accumulators is registered for cleanup
assert(accumsRegistered.size === numInternalAccums * 2)
- assert(accumsRegistered.toSet === AccumulatorContext.accumIds)
+ assert(accumsRegistered.size === AccumulatorContext.numAccums)
--- End diff --
nit: use `.toSet.size` to make it identical to the previous test.
---
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-15003] Use ConcurrentHashMap in place o...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215910436
Thanks, merging to master!
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215735899
**[Test build #57338 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57338/consoleFull)** for PR 12776 at commit [`2103175`](https://github.com/apache/spark/commit/2103175e2ab0790ae97b732859f6d01680413d84).
* This patch **fails Scala style 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: [SPARK-15003] Use ConcurrentHashMap in place o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215783114
Merged build finished. Test FAILed.
---
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-15003] Use ConcurrentHashMap in place o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215796267
**[Test build #57342 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57342/consoleFull)** for PR 12776 at commit [`c6d2a15`](https://github.com/apache/spark/commit/c6d2a15b9f77497291762a8736c3980373e2743a).
* 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: Use ConcurrentHashMap in place of HashMap for ...
Posted by tedyu <gi...@git.apache.org>.
Github user tedyu commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61576634
--- Diff: core/src/main/scala/org/apache/spark/NewAccumulator.scala ---
@@ -209,9 +210,9 @@ private[spark] object AccumulatorContext {
*/
def newId(): Long = nextId.getAndIncrement
- def numAccums: Int = synchronized(originals.size)
+ def numAccums: Int = originals.size
- def accumIds: Set[Long] = synchronized(originals.keySet().asScala.toSet)
+ def accumIds: Set[Long] = originals.keySet().asScala.toSet
--- End diff --
This is only used in InternalAccumulatorSuite.scala
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215735912
Merged build finished. Test FAILed.
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215681965
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61586664
--- Diff: core/src/test/scala/org/apache/spark/InternalAccumulatorSuite.scala ---
@@ -188,13 +188,18 @@ class InternalAccumulatorSuite extends SparkFunSuite with LocalSparkContext {
val numInternalAccums = TaskMetrics.empty.internalAccums.length
// We ran 2 stages, so we should have 2 sets of internal accumulators, 1 for each stage
assert(AccumulatorContext.numAccums === numInternalAccums * 2)
+ var cnt = 0
val accumsRegistered = sc.cleaner match {
--- End diff --
nit: we don't need the `cnt`, just do:
```
assert(accumsRegistered.size === numInternalAccums * 2)
assert(accumsRegistered.size === AccumulatorContext.numAccums)
accumsRegistered.foreach(id => assert(AccumulatorContext.get(id) != None))
```
---
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-15003] Use ConcurrentHashMap in place o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215820703
Merged build finished. Test FAILed.
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215661922
**[Test build #57325 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57325/consoleFull)** for PR 12776 at commit [`f8eb88e`](https://github.com/apache/spark/commit/f8eb88ea8ba11df955d05a52e5cf1f6037512842).
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215735133
**[Test build #57338 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57338/consoleFull)** for PR 12776 at commit [`2103175`](https://github.com/apache/spark/commit/2103175e2ab0790ae97b732859f6d01680413d84).
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61585641
--- Diff: core/src/test/scala/org/apache/spark/InternalAccumulatorSuite.scala ---
@@ -188,13 +188,18 @@ class InternalAccumulatorSuite extends SparkFunSuite with LocalSparkContext {
val numInternalAccums = TaskMetrics.empty.internalAccums.length
// We ran 2 stages, so we should have 2 sets of internal accumulators, 1 for each stage
assert(AccumulatorContext.numAccums === numInternalAccums * 2)
+ val cnt = 0
val accumsRegistered = sc.cleaner match {
- case Some(cleaner: SaveAccumContextCleaner) => cleaner.accumsRegisteredForCleanup
+ case Some(cleaner: SaveAccumContextCleaner) => {
--- End diff --
accumsRegistered is not used now. You don't need to count the number of elements, just get its size. Then you can assert for each that it isDefined rather than != None.
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by tedyu <gi...@git.apache.org>.
Github user tedyu commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61585127
--- Diff: core/src/main/scala/org/apache/spark/NewAccumulator.scala ---
@@ -209,9 +210,9 @@ private[spark] object AccumulatorContext {
*/
def newId(): Long = nextId.getAndIncrement
- def numAccums: Int = synchronized(originals.size)
+ def numAccums: Int = originals.size
- def accumIds: Set[Long] = synchronized(originals.keySet().asScala.toSet)
+ def accumIds: Set[Long] = originals.keySet().asScala.toSet
--- End diff --
I rewrote the assertions in InternalAccumulatorSuite
I think accumIds is not needed.
Once @cloud-fan confirms, I will remove accumIds.
---
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-15003] Use ConcurrentHashMap in place o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215820708
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57344/
Test FAILed.
---
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-15003] Use ConcurrentHashMap in place o...
Posted by tedyu <gi...@git.apache.org>.
Github user tedyu commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215784559
Test failure was not related to the patch:
```
[info] - analyze MetastoreRelations *** FAILED *** (483 milliseconds)
[info] 3426 did not equal 11624 (StatisticsSuite.scala:83)
[info] org.scalatest.exceptions.TestFailedException:
[info] at org.scalatest.Assertions$class.newAssertionFailedException(Assertions.scala:500)
[info] at org.scalatest.FunSuite.newAssertionFailedException(FunSuite.scala:1555)
```
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215737238
**[Test build #57339 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57339/consoleFull)** for PR 12776 at commit [`90a1432`](https://github.com/apache/spark/commit/90a143231be77ef9c3b80a81745b96074701d70a).
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215681805
**[Test build #57325 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57325/consoleFull)** for PR 12776 at commit [`f8eb88e`](https://github.com/apache/spark/commit/f8eb88ea8ba11df955d05a52e5cf1f6037512842).
* 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: [SPARK-15003] Use ConcurrentHashMap in place o...
Posted by tedyu <gi...@git.apache.org>.
Github user tedyu commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215822166
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-15003] Use ConcurrentHashMap in place o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215853017
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57347/
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: [SPARK-15003] Use ConcurrentHashMap in place o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215822834
**[Test build #57347 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57347/consoleFull)** for PR 12776 at commit [`ff8ed2c`](https://github.com/apache/spark/commit/ff8ed2c7dc656d0f9a2d739ccddf3e907972118e).
---
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-15003] Use ConcurrentHashMap in place o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215853016
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215737708
**[Test build #57339 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57339/consoleFull)** for PR 12776 at commit [`90a1432`](https://github.com/apache/spark/commit/90a143231be77ef9c3b80a81745b96074701d70a).
* This patch **fails Scala style 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: [SPARK-15003] Use ConcurrentHashMap in place o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215748341
**[Test build #57334 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57334/consoleFull)** for PR 12776 at commit [`3754218`](https://github.com/apache/spark/commit/375421882699292ef63388e70a5fc80e40dffed2).
* 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: [SPARK-15003] Use ConcurrentHashMap in place o...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/12776
---
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-15003] Use ConcurrentHashMap in place o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215748650
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215738820
**[Test build #57340 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57340/consoleFull)** for PR 12776 at commit [`cffbc13`](https://github.com/apache/spark/commit/cffbc13af3a03e0df6bdc3ea3763dd66221b839a).
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215736723
LGTM, can you create a JIRA for it?
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61581958
--- Diff: core/src/main/scala/org/apache/spark/NewAccumulator.scala ---
@@ -209,9 +210,9 @@ private[spark] object AccumulatorContext {
*/
def newId(): Long = nextId.getAndIncrement
- def numAccums: Int = synchronized(originals.size)
+ def numAccums: Int = originals.size
- def accumIds: Set[Long] = synchronized(originals.keySet().asScala.toSet)
+ def accumIds: Set[Long] = originals.keySet().asScala.toSet
--- End diff --
It still needs to be synchronized as it this iterates over the map, and that does not happen atomically. Even if you argue it doesn't happen to occur now given its usage, it's more correct.
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61586223
--- Diff: core/src/test/scala/org/apache/spark/InternalAccumulatorSuite.scala ---
@@ -188,13 +188,18 @@ class InternalAccumulatorSuite extends SparkFunSuite with LocalSparkContext {
val numInternalAccums = TaskMetrics.empty.internalAccums.length
// We ran 2 stages, so we should have 2 sets of internal accumulators, 1 for each stage
assert(AccumulatorContext.numAccums === numInternalAccums * 2)
+ var cnt = 0
val accumsRegistered = sc.cleaner match {
- case Some(cleaner: SaveAccumContextCleaner) => cleaner.accumsRegisteredForCleanup
+ case Some(cleaner: SaveAccumContextCleaner) => {
+ for (id <- cleaner.accumsRegisteredForCleanup) {
+ assert(AccumulatorContext.get(id) != None)
+ cnt = cnt + 1
+ }
+ }
case _ => Seq.empty[Long]
}
// Make sure the same set of accumulators is registered for cleanup
- assert(accumsRegistered.size === numInternalAccums * 2)
- assert(accumsRegistered.toSet === AccumulatorContext.accumIds)
+ assert(cnt === numInternalAccums * 2)
--- End diff --
add one more condition: `assert(cnt == AccumulatorContext.numAccums)`
---
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-15003] Use ConcurrentHashMap in place o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215820418
**[Test build #57344 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57344/consoleFull)** for PR 12776 at commit [`ff8ed2c`](https://github.com/apache/spark/commit/ff8ed2c7dc656d0f9a2d739ccddf3e907972118e).
* This patch **fails Spark unit 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: Use ConcurrentHashMap in place of HashMap for ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215681967
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57325/
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61551752
--- Diff: core/src/main/scala/org/apache/spark/NewAccumulator.scala ---
@@ -199,7 +199,8 @@ private[spark] object AccumulatorContext {
* TODO: Don't use a global map; these should be tied to a SparkContext (SPARK-13051).
*/
@GuardedBy("AccumulatorContext")
--- End diff --
I think this annotation is no longer needed.
But first, what's the advantage of this change overall?
---
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-15003] Use ConcurrentHashMap in place o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215783122
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57340/
Test FAILed.
---
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-15003] Use ConcurrentHashMap in place o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215776127
**[Test build #57344 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57344/consoleFull)** for PR 12776 at commit [`ff8ed2c`](https://github.com/apache/spark/commit/ff8ed2c7dc656d0f9a2d739ccddf3e907972118e).
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61551583
--- Diff: core/src/main/scala/org/apache/spark/NewAccumulator.scala ---
@@ -209,9 +210,9 @@ private[spark] object AccumulatorContext {
*/
def newId(): Long = nextId.getAndIncrement
- def numAccums: Int = synchronized(originals.size)
+ def numAccums: Int = originals.size
- def accumIds: Set[Long] = synchronized(originals.keySet().asScala.toSet)
+ def accumIds: Set[Long] = originals.keySet().asScala.toSet
--- End diff --
This still needs to be synchronized externally
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215740221
**[Test build #57342 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57342/consoleFull)** for PR 12776 at commit [`c6d2a15`](https://github.com/apache/spark/commit/c6d2a15b9f77497291762a8736c3980373e2743a).
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/12776#discussion_r61586305
--- Diff: core/src/main/scala/org/apache/spark/NewAccumulator.scala ---
@@ -209,9 +210,9 @@ private[spark] object AccumulatorContext {
*/
def newId(): Long = nextId.getAndIncrement
- def numAccums: Int = synchronized(originals.size)
+ def numAccums: Int = originals.size
- def accumIds: Set[Long] = synchronized(originals.keySet().asScala.toSet)
+ def accumIds: Set[Long] = originals.keySet().asScala.toSet
--- End diff --
yea we can remove this with your rewriting to the test.
---
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: Use ConcurrentHashMap in place of HashMap for ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215709456
**[Test build #57334 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57334/consoleFull)** for PR 12776 at commit [`3754218`](https://github.com/apache/spark/commit/375421882699292ef63388e70a5fc80e40dffed2).
---
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-15003] Use ConcurrentHashMap in place o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12776#issuecomment-215748654
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57334/
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