Skip to content
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.

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

[SPARK-13746][Tests]stop using deprecated SynchronizedSet #11580

Closed
wants to merge 6 commits into from

Conversation

wilswu
Copy link

@wilswu wilswu commented Mar 8, 2016

trait SynchronizedSet in package mutable is deprecated

toBeCleanedShuffleIds.isEmpty &&
toBeCleanedBroadcstIds.isEmpty &&
toBeCheckpointIds.isEmpty
synchronized {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't synchronizing access to these sets on the same lock.

Copy link
Author

Choose a reason for hiding this comment

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

@srowen
Fixed. Thanks!

val toBeCleanedShuffleIds = new HashSet[Int] with SynchronizedSet[Int] ++= shuffleIds
val toBeCleanedBroadcstIds = new HashSet[Long] with SynchronizedSet[Long] ++= broadcastIds
val toBeCheckpointIds = new HashSet[Long] with SynchronizedSet[Long] ++= checkpointIds
val toBeCleanedRDDIds = new HashSet[Int] ++= rddIds
Copy link
Member

Choose a reason for hiding this comment

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

Can this just be = HashSet(rddIds) and similarly for the next 4?

Copy link
Author

Choose a reason for hiding this comment

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

@srowen
I tried to change to val toBeCleanedRDDIds = HashSet(rddIds), but I got error at
toBeCleanedRDDIds -= rddId
so I will keep val toBeCleanedRDDIds = new HashSet[Int] ++= rddIds

Copy link
Member

Choose a reason for hiding this comment

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

Ah right Scala doesn't have a constructor that builds a set from a list/seq, so this makes a set of a Seq[Int]. Yes this is fine.

@srowen
Copy link
Member

srowen commented Mar 10, 2016

This needs a rebase anyway so I left a few more comments. Otherwise LGTM pending tests

@andrewor14
Copy link
Contributor

ok to test

@SparkQA
Copy link

SparkQA commented Mar 12, 2016

Test build #52968 has finished for PR 11580 at commit 6721259.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Mar 13, 2016

Test build #53024 has finished for PR 11580 at commit c1e4e41.

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

@@ -578,18 +578,27 @@ class CleanerTester(
}

private def uncleanedResourcesToString = {
val s1 = {toBeCleanedRDDIds.synchronized {
Copy link
Member

Choose a reason for hiding this comment

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

These two statements have extra braces

Copy link
Author

Choose a reason for hiding this comment

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

@srowen
Sorry, I overlooked this. Will change now. Thanks!

@SparkQA
Copy link

SparkQA commented Mar 13, 2016

Test build #53035 has finished for PR 11580 at commit bb8b83a.

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

@srowen
Copy link
Member

srowen commented Mar 14, 2016

Merged to master

@asfgit asfgit closed this in 31d069d Mar 14, 2016
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Mar 17, 2016
trait SynchronizedSet in package mutable is deprecated

Author: Wilson Wu <wilson888888888@gmail.com>

Closes apache#11580 from wilson888888888/spark-synchronizedset.
roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016
trait SynchronizedSet in package mutable is deprecated

Author: Wilson Wu <wilson888888888@gmail.com>

Closes apache#11580 from wilson888888888/spark-synchronizedset.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants