Skip to content

[SPARK-17463][Core]Make CollectionAccumulator and SetAccumulator's value can be read thread-safely #15063

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
zsxwing wants to merge 4 commits into from
Closed

[SPARK-17463][Core]Make CollectionAccumulator and SetAccumulator's value can be read thread-safely #15063

zsxwing wants to merge 4 commits into from

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Sep 12, 2016

What changes were proposed in this pull request?

Make CollectionAccumulator and SetAccumulator's value can be read thread-safely to fix the ConcurrentModificationException reported in JIRA.

How was this patch tested?

Existing tests.

@SparkQA
Copy link

SparkQA commented Sep 12, 2016

Test build #65271 has finished for PR 15063 at commit 21b6b4d.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class SetAccumulator[T] extends AccumulatorV2[T, java.util.Set[T]]

@JoshRosen
Copy link
Contributor

JoshRosen commented Sep 12, 2016

I think that we may also want to do this for BlockStatusesAccumulator

JoshRosen
JoshRosen reviewed Sep 12, 2016
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/debug/package.scala
class SetAccumulator[T] extends AccumulatorV2[T, HashSet[T]] {
private val _set = new HashSet[T]()
class SetAccumulator[T] extends AccumulatorV2[T, java.util.Set[T]] {
private val _set = Collections.synchronizedSet(new java.util.HashSet[T]())
Copy link
Contributor

@JoshRosen JoshRosen Sep 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use Collections.synchronized*, will serialization of those objects also be thread-safe (i.e. will writeObject synchronize properly)? What about if Kryo is used?

Copy link
Member Author

@zsxwing zsxwing Sep 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Java serialization, it's synchronized. See: http://www.grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/Collections.java#2080

Do we use Kryo to serialize Heartbeat?

Copy link
Contributor

@JoshRosen JoshRosen Sep 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it looks like NettyRpcEnv.serialize is hardcoded to use a JavaSerializer instance.

@SparkQA
Copy link

SparkQA commented Sep 12, 2016

Test build #65270 has finished for PR 15063 at commit 4b8c277.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 12, 2016

Test build #3255 has finished for PR 15063 at commit 21b6b4d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class SetAccumulator[T] extends AccumulatorV2[T, java.util.Set[T]]

@SparkQA
Copy link

SparkQA commented Sep 12, 2016

Test build #65272 has finished for PR 15063 at commit 5a7183b.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor

JoshRosen commented Sep 12, 2016

Jenkins, retest this please.

(Retesting so MiMa can run again)

@SparkQA
Copy link

SparkQA commented Sep 12, 2016

Test build #65281 has finished for PR 15063 at commit 0da2f9b.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 13, 2016

Test build #65287 has finished for PR 15063 at commit 021c628.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor

JoshRosen commented Sep 14, 2016

LGTM, so I'm going to merge this into master and branch-2.0.

asfgit pushed a commit that referenced this pull request Sep 14, 2016
@zsxwing @JoshRosen
[SPARK-17463][CORE] Make CollectionAccumulator and SetAccumulator's v…
fffcec9 
…alue can be read thread-safely

## What changes were proposed in this pull request?

Make CollectionAccumulator and SetAccumulator's value can be read thread-safely to fix the ConcurrentModificationException reported in [JIRA](https://issues.apache.org/jira/browse/SPARK-17463).

## How was this patch tested?

Existing tests.

Author: Shixiong Zhu <shixiong@databricks.com>

Closes #15063 from zsxwing/SPARK-17463.

(cherry picked from commit e33bfae)
Signed-off-by: Josh Rosen <joshrosen@databricks.com>
@asfgit asfgit closed this in e33bfae Sep 14, 2016
@zsxwing zsxwing deleted the SPARK-17463 branch September 14, 2016 20:59
wgtmac pushed a commit to wgtmac/spark that referenced this pull request Sep 19, 2016
@zsxwing @wgtmac
[SPARK-17463][CORE] Make CollectionAccumulator and SetAccumulator's v…
e5814cc 
…alue can be read thread-safely

## What changes were proposed in this pull request?

Make CollectionAccumulator and SetAccumulator's value can be read thread-safely to fix the ConcurrentModificationException reported in [JIRA](https://issues.apache.org/jira/browse/SPARK-17463).

## How was this patch tested?

Existing tests.

Author: Shixiong Zhu <shixiong@databricks.com>

Closes apache#15063 from zsxwing/SPARK-17463.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoshRosen JoshRosen JoshRosen left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zsxwing @SparkQA @JoshRosen