You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by seyfe <gi...@git.apache.org> on 2016/10/06 01:40:05 UTC
[GitHub] spark pull request #15371: [SPARK-17463] [Core] TaskInfoToJson should map us...
GitHub user seyfe opened a pull request:
https://github.com/apache/spark/pull/15371
[SPARK-17463] [Core] TaskInfoToJson should map using List(immutable) rather than ListBuffer(mutable)
## What changes were proposed in this pull request?
taskInfo.accumulables is ListBuffer and causing java.util.ConcurrentModificationException.
Converted to List before doing map will fix the concurrency issue here.
## How was this patch tested?
This is the stack trace for this exception:
`
java.util.ConcurrentModificationException
at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901)
at java.util.ArrayList$Itr.next(ArrayList.java:851)
at scala.collection.convert.Wrappers$JIteratorWrapper.next(Wrappers.scala:43)
at scala.collection.Iterator$class.foreach(Iterator.scala:893)
at scala.collection.AbstractIterator.foreach(Iterator.scala:1336)
at scala.collection.IterableLike$class.foreach(IterableLike.scala:72)
at scala.collection.AbstractIterable.foreach(Iterable.scala:54)
at scala.collection.generic.Growable$class.$plus$plus$eq(Growable.scala:59)
at scala.collection.mutable.ListBuffer.$plus$plus$eq(ListBuffer.scala:183)
at scala.collection.mutable.ListBuffer.$plus$plus$eq(ListBuffer.scala:45)
at scala.collection.TraversableLike$class.to(TraversableLike.scala:590)
at scala.collection.AbstractTraversable.to(Traversable.scala:104)
at scala.collection.TraversableOnce$class.toList(TraversableOnce.scala:294)
at scala.collection.AbstractTraversable.toList(Traversable.scala:104)
at org.apache.spark.util.JsonProtocol$.accumValueToJson(JsonProtocol.scala:314)
at org.apache.spark.util.JsonProtocol$$anonfun$accumulableInfoToJson$5.apply(JsonProtocol.scala:291)
at org.apache.spark.util.JsonProtocol$$anonfun$accumulableInfoToJson$5.apply(JsonProtocol.scala:291)
at scala.Option.map(Option.scala:146)
at org.apache.spark.util.JsonProtocol$.accumulableInfoToJson(JsonProtocol.scala:291)
at org.apache.spark.util.JsonProtocol$$anonfun$taskInfoToJson$12.apply(JsonProtocol.scala:283)
at org.apache.spark.util.JsonProtocol$$anonfun$taskInfoToJson$12.apply(JsonProtocol.scala:283)
at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
at scala.collection.immutable.List.foreach(List.scala:381)
at scala.collection.generic.TraversableForwarder$class.foreach(TraversableForwarder.scala:35)
at scala.collection.mutable.ListBuffer.foreach(ListBuffer.scala:45)
at scala.collection.TraversableLike$class.map(TraversableLike.scala:234)
at scala.collection.AbstractTraversable.map(Traversable.scala:104)
at org.apache.spark.util.JsonProtocol$.taskInfoToJson(JsonProtocol.scala:283)
at org.apache.spark.util.JsonProtocol$.taskEndToJson(JsonProtocol.scala:145)
at org.apache.spark.util.JsonProtocol$.sparkEventToJson(JsonProtocol.scala:76)
`
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/seyfe/spark race_cond_jsonprotocal
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/15371.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 #15371
----
commit 07d9a8b26469dd6becc36f2dc9e17a58c53ff4e6
Author: Ergin Seyfe <es...@fb.com>
Date: 2016-10-06T01:34:57Z
Map for List rather than ListBuffer
----
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66537 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66537/consoleFull)** for PR 15371 at commit [`a5d3eb7`](https://github.com/apache/spark/commit/a5d3eb76c1407f4ba22bbd68a30df8527d966bff).
---
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82467368
--- Diff: core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala ---
@@ -344,15 +338,20 @@ private[spark] class BlockStatusesAccumulator
override def add(v: (BlockId, BlockStatus)): Unit = _seq.add(v)
override def merge(
- other: AccumulatorV2[(BlockId, BlockStatus), java.util.List[(BlockId, BlockStatus)]]): Unit = {
+ other: AccumulatorV2[(BlockId, BlockStatus), List[(BlockId, BlockStatus)]]): Unit = {
other match {
- case o: BlockStatusesAccumulator => _seq.addAll(o.value)
+ case o: BlockStatusesAccumulator => o.value.foreach(_seq.add)
case _ => throw new UnsupportedOperationException(
s"Cannot merge ${this.getClass.getName} with ${other.getClass.getName}")
}
}
- override def value: java.util.List[(BlockId, BlockStatus)] = _seq
+ // `asScala` accesses the internal values using `java.util.Iterator` so needs to be synchronized
+ override def value: List[(BlockId, BlockStatus)] = {
+ _seq.synchronized {
--- End diff --
`_seq.synchronized` is wrong. `Collections.synchronizedList` uses its internal `mutex` to lock instead of `this`.
Why changes them to Scala List? Just change this one to `java.util.Collections.unmodifiableList(new ArrayList[(BlockId, BlockStatus)](_seq))` should be enough.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66632 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66632/consoleFull)** for PR 15371 at commit [`da2311a`](https://github.com/apache/spark/commit/da2311a5a0cee356169bed1a940bcb5bb0c87b26).
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66627 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66627/consoleFull)** for PR 15371 at commit [`da2311a`](https://github.com/apache/spark/commit/da2311a5a0cee356169bed1a940bcb5bb0c87b26).
* 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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82559538
--- Diff: project/MimaExcludes.scala ---
@@ -55,7 +55,11 @@ object MimaExcludes {
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.getFunction"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.databaseExists"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.tableExists"),
- ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.functionExists")
+ ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.functionExists"),
+ // [SPARK-17816] Fix ConcurrentModificationException issue in BlockStatusesAccumulator
+ ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.spark.util.AccumulatorV2.merge"),
--- End diff --
Hm, but this class didn't have any change in its API, not at this point. I can't see why it would still be needed. In fact there's a merge conflict here anyway, can you just try removing it and see if it passes?
Looking strong to me otherwise, great cleanup.
---
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 issue #15371: [SPARK-17463] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66425 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66425/consoleFull)** for PR 15371 at commit [`1934dc9`](https://github.com/apache/spark/commit/1934dc9148541869313ed6237afc4e7b014ff30b).
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66544/
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66534 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66534/consoleFull)** for PR 15371 at commit [`97977ee`](https://github.com/apache/spark/commit/97977ee51ef1ca7423b571ad639702fa19884de4).
* This patch **fails to build**.
* 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/15371
You can find the following errors on Jenkins:
```
[error] * abstract method merge(org.apache.spark.util.AccumulatorV2)Unit in class org.apache.spark.util.AccumulatorV2 does not have a correspondent in current version
[error] filter with: ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.spark.util.AccumulatorV2.merge")
[error] * abstract method isZero()Boolean in class org.apache.spark.util.AccumulatorV2 does not have a correspondent in current version
[error] filter with: ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.spark.util.AccumulatorV2.isZero")
[error] * abstract method reset()Unit in class org.apache.spark.util.AccumulatorV2 does not have a correspondent in current version
[error] filter with: ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.spark.util.AccumulatorV2.reset")
```
Just add these exclude rules to `project/MimaExcludes.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 issue #15371: [SPARK-17463] [Core] Fix ConcurrentModificationException...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/15371
One comment: once you figured out the proper fix, please add some comments inline so they don't get accidentally removed in the future.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66584 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66584/consoleFull)** for PR 15371 at commit [`24a3313`](https://github.com/apache/spark/commit/24a3313d7e46a2bae8f56939b8d7100f65b13ef2).
* 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66473 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66473/consoleFull)** for PR 15371 at commit [`5380aff`](https://github.com/apache/spark/commit/5380aff7d871b03577e7cefb40c616ab747879d5).
* This patch **fails to build**.
* 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
@zsxwing . `JArray(v.asInstanceOf[java.util.List[(BlockId, BlockStatus)]].asScala.toList.map` this is the line of code which is causing ConcurrentModificationException. And you can see that from call stack. This is coming from BlockStatusesAccumulator. Am I missing something 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 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/15371
@seyfe I think we can remove `BlockStatusesAccumulator` and just use `private val _updatedBlockStatuses = new CollectionAccumulator[(BlockId, BlockStatus)]` instead. `BlockStatusesAccumulator` doesn't provide more functions than `CollectionAccumulator`. Sorry that I didn't find that early.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15371
I see the new copy (of course or else this wouldn't help) but where is a copy removed? I'm probably overlooking it. A `toList` call was moved, which we discussed above briefly. I still think it's all fine, just worth double-checking both of our logics.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66669 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66669/consoleFull)** for PR 15371 at commit [`5e00dc3`](https://github.com/apache/spark/commit/5e00dc324483eb3b7dbf8f2771c651876f529964).
* 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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82679357
--- Diff: core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala ---
@@ -444,7 +444,9 @@ class CollectionAccumulator[T] extends AccumulatorV2[T, java.util.List[T]] {
override def copy(): CollectionAccumulator[T] = {
val newAcc = new CollectionAccumulator[T]
- newAcc._list.addAll(_list)
+ _list.synchronized {
--- End diff --
@mridulm , I don't think that your statement is correct. `SynchronizedCollection` is protected by
` final Object mutex; // Object on which to synchronize
`
If you look at the constructor you will see that
` mutex = this;
`
Does that make sense 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 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
@zsxwing. `BlockStatusesAccumulator` is using synchronizedList which is `_seq` for concurrency. But when the `value` method is executed and the output is used by `asScala.toList` it caused the `ConcurrentModificationException`.
`Collections.synchronizedList` suggests using synchronized keyword when using its iterator which is done by asScala method. Does that make sense?
---
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 issue #15371: [SPARK-17463] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66425 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66425/consoleFull)** for PR 15371 at commit [`1934dc9`](https://github.com/apache/spark/commit/1934dc9148541869313ed6237afc4e7b014ff30b).
* 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66473 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66473/consoleFull)** for PR 15371 at commit [`5380aff`](https://github.com/apache/spark/commit/5380aff7d871b03577e7cefb40c616ab747879d5).
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66611 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66611/consoleFull)** for PR 15371 at commit [`1640e0e`](https://github.com/apache/spark/commit/1640e0ea4085c5c2ff14cdea1cdd1363e8b1b6ca).
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66534/
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/15371
LGTM. Thanks! Merging to master and 2.0.
---
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 issue #15371: [SPARK-17463] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17463] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66425/
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66584 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66584/consoleFull)** for PR 15371 at commit [`24a3313`](https://github.com/apache/spark/commit/24a3313d7e46a2bae8f56939b8d7100f65b13ef2).
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66541 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66541/consoleFull)** for PR 15371 at commit [`2d5656c`](https://github.com/apache/spark/commit/2d5656c20d5314279694ae7be22096cb95589528).
* This patch **fails MiMa 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
Thanks @zsxwing.
Here is the PR for branch-2.0
https://github.com/apache/spark/pull/15425
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/15371
> @zsxwing. BlockStatusesAccumulator is using synchronizedList which is _seq for concurrency. But when the value method is executed and the output is used by asScala.toList it caused the ConcurrentModificationException.
>
> Collections.synchronizedList suggests using synchronized keyword when using its iterator which is done by asScala method. Does that make sense?
@seyfe The comment you are deleting explains why it's safe: the driver doesn't modify `BlockStatusesAccumulator`.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66534 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66534/consoleFull)** for PR 15371 at commit [`97977ee`](https://github.com/apache/spark/commit/97977ee51ef1ca7423b571ad639702fa19884de4).
---
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82468147
--- Diff: core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala ---
@@ -344,15 +338,20 @@ private[spark] class BlockStatusesAccumulator
override def add(v: (BlockId, BlockStatus)): Unit = _seq.add(v)
override def merge(
- other: AccumulatorV2[(BlockId, BlockStatus), java.util.List[(BlockId, BlockStatus)]]): Unit = {
+ other: AccumulatorV2[(BlockId, BlockStatus), List[(BlockId, BlockStatus)]]): Unit = {
other match {
- case o: BlockStatusesAccumulator => _seq.addAll(o.value)
+ case o: BlockStatusesAccumulator => o.value.foreach(_seq.add)
case _ => throw new UnsupportedOperationException(
s"Cannot merge ${this.getClass.getName} with ${other.getClass.getName}")
}
}
- override def value: java.util.List[(BlockId, BlockStatus)] = _seq
+ // `asScala` accesses the internal values using `java.util.Iterator` so needs to be synchronized
+ override def value: List[(BlockId, BlockStatus)] = {
+ _seq.synchronized {
--- End diff --
Thanks for the review @zsxwing . I checked the java doc and it says that getting iterator is not thread safe and suggests below usage. That's why I did `_seq.synchronized`
https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html
```
List list = Collections.synchronizedList(new ArrayList());
...
synchronized (list) {
Iterator i = list.iterator(); // Must be in synchronized block
while (i.hasNext())
foo(i.next());
}
```
Regarding your second questions, `JsonProtocal` is using it as Scala collection that is why I converted it to a Scala collection so we won't need to convert again.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66539/
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82521015
--- Diff: core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala ---
@@ -444,7 +444,7 @@ class CollectionAccumulator[T] extends AccumulatorV2[T, java.util.List[T]] {
override def copy(): CollectionAccumulator[T] = {
val newAcc = new CollectionAccumulator[T]
- newAcc._list.addAll(_list)
+ newAcc.merge(this)
--- End diff --
Yeah I see why this is probably worth fixing in the base class too. The only concern I have is that `merge` causes a copy of the argument's list, when here we could get away with `_list.synchronized { newAcc._list.addAll(_list) }`? It would also avoid a tiny behavior change, that subclasses would now find copy calls merge (though I don't know of a reason that would be a problem now.) We could extend the same logic to `merge` to avoid calling `value` but it would require holding locks on two lists at once and there's an opening for a deadlock there, I think, that I don't want to mess with.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
I also want to point out that below is the core part of fix. Rest of the code changes are side-effects of it.
```
+ // `asScala` accesses the internal values using `java.util.Iterator` so needs to be synchronized
+ override def value: List[(BlockId, BlockStatus)] = {
+ _seq.synchronized {
+ _seq.asScala.toList
+ }
+ }
```
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66539 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66539/consoleFull)** for PR 15371 at commit [`a5d3eb7`](https://github.com/apache/spark/commit/a5d3eb76c1407f4ba22bbd68a30df8527d966bff).
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/15371
There are some conflicts with 2.0. @seyfe could you submit a PR for branch-2.0, please? Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82544528
--- Diff: core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala ---
@@ -444,7 +444,9 @@ class CollectionAccumulator[T] extends AccumulatorV2[T, java.util.List[T]] {
override def copy(): CollectionAccumulator[T] = {
val newAcc = new CollectionAccumulator[T]
- newAcc._list.addAll(_list)
+ _list.synchronized {
--- End diff --
Good 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 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66632 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66632/consoleFull)** for PR 15371 at commit [`da2311a`](https://github.com/apache/spark/commit/da2311a5a0cee356169bed1a940bcb5bb0c87b26).
* 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66541/
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66541 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66541/consoleFull)** for PR 15371 at commit [`2d5656c`](https://github.com/apache/spark/commit/2d5656c20d5314279694ae7be22096cb95589528).
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/15371
@seyfe I'm taking my words back. Yea, `BlockStatusesAccumulator.merge` will be called in driver.
---
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82309886
--- Diff: core/src/main/scala/org/apache/spark/util/JsonProtocol.scala ---
@@ -281,7 +281,7 @@ private[spark] object JsonProtocol {
("Finish Time" -> taskInfo.finishTime) ~
("Failed" -> taskInfo.failed) ~
("Killed" -> taskInfo.killed) ~
- ("Accumulables" -> JArray(taskInfo.accumulables.map(accumulableInfoToJson).toList))
+ ("Accumulables" -> JArray(taskInfo.accumulables.toList.map(accumulableInfoToJson)))
--- End diff --
This wasn't the root cause but it's something nice to have. If you prefer, I can revert this line.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82488129
--- Diff: project/MimaExcludes.scala ---
@@ -833,6 +833,11 @@ object MimaExcludes {
// [SPARK-12221] Add CPU time to metrics
ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.status.api.v1.TaskMetrics.this"),
ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.status.api.v1.TaskMetricDistributions.this")
+ ) ++ Seq(
--- End diff --
They should be in `lazy val v21excludes = v20excludes`. Master is supposed to be released as 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 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/15371
retest 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66535 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66535/consoleFull)** for PR 15371 at commit [`9572ebd`](https://github.com/apache/spark/commit/9572ebda3d72257069bf8fc6be7cc15018ba612f).
* This patch **fails to build**.
* 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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82670700
--- Diff: core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala ---
@@ -444,7 +444,9 @@ class CollectionAccumulator[T] extends AccumulatorV2[T, java.util.List[T]] {
override def copy(): CollectionAccumulator[T] = {
val newAcc = new CollectionAccumulator[T]
- newAcc._list.addAll(_list)
+ _list.synchronized {
--- End diff --
I dont think this helps since all modifications to _list is happening without it being synchronized. What am I missing 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 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 #15371: [SPARK-17463] [Core] Fix ConcurrentModificationEx...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82132969
--- Diff: core/src/main/scala/org/apache/spark/util/JsonProtocol.scala ---
@@ -312,11 +312,15 @@ private[spark] object JsonProtocol {
// We only have 3 kind of internal accumulator types, so if it's not int or long, it must be
// the blocks accumulator, whose type is `java.util.List[(BlockId, BlockStatus)]`
case v =>
- JArray(v.asInstanceOf[java.util.List[(BlockId, BlockStatus)]].asScala.toList.map {
- case (id, status) =>
- ("Block ID" -> id.toString) ~
- ("Status" -> blockStatusToJson(status))
- })
+ val blockAccumulator =
--- End diff --
I don't see why this would help. You add a wrapper, but, synchronizing your local access to it doesn't do anything because nothing else is synchronizing on it.
PS can the `List[(BlockId,BlockStatus)]` type just be part of the match predicate?
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66669 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66669/consoleFull)** for PR 15371 at commit [`5e00dc3`](https://github.com/apache/spark/commit/5e00dc324483eb3b7dbf8f2771c651876f529964).
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66627/
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66632/
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/15371
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66584/
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82336487
--- Diff: core/src/main/scala/org/apache/spark/util/JsonProtocol.scala ---
@@ -281,7 +281,7 @@ private[spark] object JsonProtocol {
("Finish Time" -> taskInfo.finishTime) ~
("Failed" -> taskInfo.failed) ~
("Killed" -> taskInfo.killed) ~
- ("Accumulables" -> JArray(taskInfo.accumulables.map(accumulableInfoToJson).toList))
+ ("Accumulables" -> JArray(taskInfo.accumulables.toList.map(accumulableInfoToJson)))
--- End diff --
Seems OK if it's related cleanup, and potentially helps a closely related manifestation
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66537 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66537/consoleFull)** for PR 15371 at commit [`a5d3eb7`](https://github.com/apache/spark/commit/a5d3eb76c1407f4ba22bbd68a30df8527d966bff).
* This patch **fails MiMa 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82500617
--- Diff: core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala ---
@@ -326,36 +324,13 @@ private[spark] object TaskMetrics extends Logging {
private[spark] class BlockStatusesAccumulator
- extends AccumulatorV2[(BlockId, BlockStatus), java.util.List[(BlockId, BlockStatus)]] {
- private val _seq = Collections.synchronizedList(new ArrayList[(BlockId, BlockStatus)]())
-
- override def isZero(): Boolean = _seq.isEmpty
+ extends CollectionAccumulator[(BlockId, BlockStatus)] {
override def copyAndReset(): BlockStatusesAccumulator = new BlockStatusesAccumulator
override def copy(): BlockStatusesAccumulator = {
val newAcc = new BlockStatusesAccumulator
- newAcc._seq.addAll(_seq)
+ newAcc._list.addAll(_list)
--- End diff --
Don't you just want to call merge() here? it's better not to expose the internal representation even to subclasses if possible, because we're exposing it permanently to everyone.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82509108
--- Diff: core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala ---
@@ -326,36 +324,13 @@ private[spark] object TaskMetrics extends Logging {
private[spark] class BlockStatusesAccumulator
- extends AccumulatorV2[(BlockId, BlockStatus), java.util.List[(BlockId, BlockStatus)]] {
- private val _seq = Collections.synchronizedList(new ArrayList[(BlockId, BlockStatus)]())
-
- override def isZero(): Boolean = _seq.isEmpty
+ extends CollectionAccumulator[(BlockId, BlockStatus)] {
override def copyAndReset(): BlockStatusesAccumulator = new BlockStatusesAccumulator
override def copy(): BlockStatusesAccumulator = {
val newAcc = new BlockStatusesAccumulator
- newAcc._seq.addAll(_seq)
+ newAcc._list.addAll(_list)
--- End diff --
Good point. I will do the proposed change. I am also going to change the base class (`CollectionAccumulator`) because behavior of `addAll` is undefined if the specified collection is modified and `merge()` is working on immutable copy when calling `addAll`.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/15371
@seyfe The issue is `TaskInfo.accumulables` is accessed in multiple threads without synchronization. `TaskMetrics.updatedBlockStatuses` is fine.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66539 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66539/consoleFull)** for PR 15371 at commit [`a5d3eb7`](https://github.com/apache/spark/commit/a5d3eb76c1407f4ba22bbd68a30df8527d966bff).
* This patch **fails MiMa 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
Hi @zsxwing ,
The test failed with below error. I don't think that it's related with my change. Should we just re-run the test or do you have any suggestion?
```
java.lang.RuntimeException: spark-core: Binary compatibility check failed!
[error] (core/*:mimaReportBinaryIssues) spark-core: Binary compatibility check 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82528765
--- Diff: core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala ---
@@ -444,7 +444,7 @@ class CollectionAccumulator[T] extends AccumulatorV2[T, java.util.List[T]] {
override def copy(): CollectionAccumulator[T] = {
val newAcc = new CollectionAccumulator[T]
- newAcc._list.addAll(_list)
+ newAcc.merge(this)
--- End diff --
I don't have a strong opinion so let me change `merge()` with `synchronized` block as you suggested.
I also propose changing `BlockStatusesAccumulator` to use `setValue()` rather than `merge()` to avoid extra type matching.
---
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82521001
--- Diff: project/MimaExcludes.scala ---
@@ -55,7 +55,11 @@ object MimaExcludes {
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.getFunction"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.databaseExists"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.tableExists"),
- ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.functionExists")
+ ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.functionExists"),
+ // [SPARK-17816] Fix ConcurrentModificationException issue in BlockStatusesAccumulator
+ ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.spark.util.AccumulatorV2.merge"),
--- End diff --
I don't think this change should be necessary?
---
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 issue #15371: [SPARK-17463] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
Hi @zsxwing. I have a fix ready and testing it now. I will create a new ticket and send an updated PR today.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66473/
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82488439
--- Diff: project/MimaExcludes.scala ---
@@ -833,6 +833,11 @@ object MimaExcludes {
// [SPARK-12221] Add CPU time to metrics
ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.status.api.v1.TaskMetrics.this"),
ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.status.api.v1.TaskMetricDistributions.this")
+ ) ++ Seq(
--- End diff --
Sure, I update 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66535 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66535/consoleFull)** for PR 15371 at commit [`9572ebd`](https://github.com/apache/spark/commit/9572ebda3d72257069bf8fc6be7cc15018ba612f).
---
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 issue #15371: [SPARK-17463] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
Unfortunately, this PR doesn't fix the java.util.ConcurrentModificationException. I can still repro it. I will spend more time on it tomorrow morning.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
@zsxwing , I built it and the test pipelines works fine. So fix is good. But I don't know how to fix the MiMa tests. Would you mind helping me 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 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
I don't know if it's related but I found a bug in the last iteration. We also need to override copy and copyAndReset methods. Otherwise it throws java.lang.ClassCastException error.
---
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 #15371: [SPARK-17463] [Core] Fix ConcurrentModificationEx...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82133279
--- Diff: core/src/main/scala/org/apache/spark/util/JsonProtocol.scala ---
@@ -281,7 +281,7 @@ private[spark] object JsonProtocol {
("Finish Time" -> taskInfo.finishTime) ~
("Failed" -> taskInfo.failed) ~
("Killed" -> taskInfo.killed) ~
- ("Accumulables" -> JArray(taskInfo.accumulables.map(accumulableInfoToJson).toList))
+ ("Accumulables" -> JArray(taskInfo.accumulables.toList.map(accumulableInfoToJson)))
--- End diff --
What does this do -- just puts a copy before the work of mapping? I could see how that would tend to help.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66611/
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66545/
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66545 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66545/consoleFull)** for PR 15371 at commit [`1526ea6`](https://github.com/apache/spark/commit/1526ea6ffe4ec2a9ef2b0f6d7ec9261c6be06b8e).
---
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82645595
--- Diff: project/MimaExcludes.scala ---
@@ -55,7 +55,11 @@ object MimaExcludes {
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.getFunction"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.databaseExists"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.tableExists"),
- ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.functionExists")
+ ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.functionExists"),
+ // [SPARK-17816] Fix ConcurrentModificationException issue in BlockStatusesAccumulator
+ ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.spark.util.AccumulatorV2.merge"),
--- End diff --
Sure, let me try again.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66537/
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66545 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66545/consoleFull)** for PR 15371 at commit [`1526ea6`](https://github.com/apache/spark/commit/1526ea6ffe4ec2a9ef2b0f6d7ec9261c6be06b8e).
* 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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82678685
--- Diff: project/MimaExcludes.scala ---
@@ -55,7 +55,11 @@ object MimaExcludes {
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.getFunction"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.databaseExists"),
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.tableExists"),
- ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.functionExists")
+ ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.sql.catalog.Catalog.functionExists"),
+ // [SPARK-17816] Fix ConcurrentModificationException issue in BlockStatusesAccumulator
+ ProblemFilters.exclude[DirectAbstractMethodProblem]("org.apache.spark.util.AccumulatorV2.merge"),
--- End diff --
@srowen, you were right. It worked this time.
---
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 #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82683704
--- Diff: core/src/main/scala/org/apache/spark/util/AccumulatorV2.scala ---
@@ -444,7 +444,9 @@ class CollectionAccumulator[T] extends AccumulatorV2[T, java.util.List[T]] {
override def copy(): CollectionAccumulator[T] = {
val newAcc = new CollectionAccumulator[T]
- newAcc._list.addAll(_list)
+ _list.synchronized {
--- End diff --
You are right, in this codepath, it is indeed protected by mutex = 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 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 issue #15371: [SPARK-17463] [Core] TaskInfoToJson should map using Lis...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15371: [SPARK-17816] [Core] Fix ConcurrentModificationEx...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on a diff in the pull request:
https://github.com/apache/spark/pull/15371#discussion_r82470766
--- Diff: core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala ---
@@ -344,15 +338,20 @@ private[spark] class BlockStatusesAccumulator
override def add(v: (BlockId, BlockStatus)): Unit = _seq.add(v)
override def merge(
- other: AccumulatorV2[(BlockId, BlockStatus), java.util.List[(BlockId, BlockStatus)]]): Unit = {
+ other: AccumulatorV2[(BlockId, BlockStatus), List[(BlockId, BlockStatus)]]): Unit = {
other match {
- case o: BlockStatusesAccumulator => _seq.addAll(o.value)
+ case o: BlockStatusesAccumulator => o.value.foreach(_seq.add)
case _ => throw new UnsupportedOperationException(
s"Cannot merge ${this.getClass.getName} with ${other.getClass.getName}")
}
}
- override def value: java.util.List[(BlockId, BlockStatus)] = _seq
+ // `asScala` accesses the internal values using `java.util.Iterator` so needs to be synchronized
+ override def value: List[(BlockId, BlockStatus)] = {
+ _seq.synchronized {
--- End diff --
Regarding #2, I take a look at `CollectionAccumulator` as well and it seems like a good idea. Let me give it a try.
---
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 issue #15371: [SPARK-17463] [Core] Fix ConcurrentModificationException...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/15371
@seyfe Thanks for reporting this one. Actually, it's different from SPARK-17463. Could you create a new ticket for this issue, please? The cause is we send a mutable TaskInfo to listeners but we may still update TaskInfo's fields (e.g., accumulables) in another thread... Ideally, all events sent to the listeners should be immutable.
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66544 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66544/consoleFull)** for PR 15371 at commit [`4a03082`](https://github.com/apache/spark/commit/4a030820152b6d91bfddf60d4a1d8c0f2ece046c).
---
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 issue #15371: [SPARK-17463] [Core] Fix ConcurrentModificationException...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/15371
ok to 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
Hi @srowen , this is the part that I removed extra copy operation. I changed this line because this conversion is already done by BlockStatusesAccumulator.
`JArray(v.asInstanceOf[java.util.List[(BlockId, BlockStatus)]].asScala.toList.map {`
with
`JArray(v.map {`
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66544 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66544/consoleFull)** for PR 15371 at commit [`4a03082`](https://github.com/apache/spark/commit/4a030820152b6d91bfddf60d4a1d8c0f2ece046c).
* 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66669/
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
Hi @srowen,
This PR doesn't introduce any extra data copy operations. It moves the data copy code from `JsonProtocol:accumValueToJson` method to `BlockStatusesAccumulator:value` method so it can be called inside the synchronized block. But let me try to answer your question.
I checked the data size using 3 different pipelines. 99% of the time ArrayList has less than 4 items. There was one of case where it maxed at 4000 items but that was less than 1% of the time. I ran my test with 4000 executors, I think that is why this 4000 number came up.
I debated other options as well. Moving Json serialization into `BlockStatusesAccumulator` would help but I didn't want to mix match these classes. I considered introducing Akka Actors for thread safety but I think that would be an overkill in this scenario since data size is small. That is why I proposed this solution but I would love to hear your proposals.
I don't know the answer for the second part of your question (below), but existing behavior is not changed. Only change is that we can convert ArrayList to a Scala List inside a synchronized block so we won't get `ConcurrentModificationException` . If we do need to rely on observing changes, I think we do need to have a bigger change. @zsxwing, would you please comment on that part?
> nothing is actually relying on observing changes to the underlying data
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66611 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66611/consoleFull)** for PR 15371 at commit [`1640e0e`](https://github.com/apache/spark/commit/1640e0ea4085c5c2ff14cdea1cdd1363e8b1b6ca).
* 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15371
**[Test build #66627 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/66627/consoleFull)** for PR 15371 at commit [`da2311a`](https://github.com/apache/spark/commit/da2311a5a0cee356169bed1a940bcb5bb0c87b26).
---
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15371
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/66535/
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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15371
@zsxwing , I think that is a good idea. I search it and that is the only place we use `BlockStatusesAccumulator`. Let me remove 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 issue #15371: [SPARK-17816] [Core] Fix ConcurrentModificationException...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the issue:
https://github.com/apache/spark/pull/15371
retest 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