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-13165][STREAMING] Replace deprecated synchronizedBuffer in streaming #11067

Conversation

holdenk
Copy link
Contributor

@holdenk holdenk commented Feb 3, 2016

Building with Scala 2.11 results in the warning trait SynchronizedBuffer in package mutable is deprecated: Synchronization via traits is deprecated as it is inherently unreliable. Consider java.util.concurrent.ConcurrentLinkedQueue as an alternative - we already use ConcurrentLinkedQueue elsewhere so lets replace it.

Some notes about how behaviour is different for reviewers:
The Seq from a SynchronizedBuffer that was implicitly converted would continue to receive updates - however when we do the same conversion explicitly on the ConcurrentLinkedQueue this isn't the case. Hence changing some of the (internal & test) APIs to pass an Iterable. toSeq is safe to use if there are no more updates.

@SparkQA
Copy link

SparkQA commented Feb 3, 2016

Test build #50706 has finished for PR 11067 at commit 9dfb313.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@holdenk
Copy link
Contributor Author

holdenk commented Feb 3, 2016

jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Feb 4, 2016

Test build #50708 has finished for PR 11067 at commit 716e4b3.

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

@SparkQA
Copy link

SparkQA commented Feb 4, 2016

Test build #50707 has finished for PR 11067 at commit 716e4b3.

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

@holdenk holdenk changed the title [SPARK-13165][STREAMING][WIP] Replace deprecated synchronizedBuffer in streaming [SPARK-13165][STREAMING] Replace deprecated synchronizedBuffer in streaming Feb 4, 2016
@holdenk
Copy link
Contributor Author

holdenk commented Feb 4, 2016

cc @srowen

@andrewor14
Copy link
Contributor

also @zsxwing

for (i <- 0 until output.size) {
assert(output(i) === expectedOutput(i))
}
output.zipWithIndex.foreach{case (e, i) => assert(e == expectedOutput(i))}
Copy link
Member

Choose a reason for hiding this comment

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

Does this need spaces around braces to pass style checks? And === vs ==?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in the latest scalatest == is compiled as ===

@srowen
Copy link
Member

srowen commented Feb 5, 2016

Minor questions; seems fine. It's actually the same simple changes in many files and mostly touches tests

@holdenk
Copy link
Contributor Author

holdenk commented Feb 5, 2016

@srowen simplified those two places in the test code :)

@SparkQA
Copy link

SparkQA commented Feb 5, 2016

Test build #50836 has finished for PR 11067 at commit 605bb84.

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

@holdenk
Copy link
Contributor Author

holdenk commented Feb 7, 2016

@tdas want to take a look or since its mostly tests is that not needed?

@holdenk
Copy link
Contributor Author

holdenk commented Feb 7, 2016

@tedyu I've cherry-picked in your DirectKafkaStreamSuite changes let me know if that looks ok to you.

@tedyu
Copy link
Contributor

tedyu commented Feb 7, 2016

It's nice of you to put related changes under one PR.

@SparkQA
Copy link

SparkQA commented Feb 7, 2016

Test build #50905 has finished for PR 11067 at commit 2567ef0.

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

@srowen
Copy link
Member

srowen commented Feb 8, 2016

Seems OK to me. This is all of the occurrences then? so we could add a scalastyle rule banning SynchronizedBuffer after this?

@holdenk
Copy link
Contributor Author

holdenk commented Feb 8, 2016

Yah @tedyu has a follow up PR that adds the scalastyle rule ready to rebase once this gets merged in.

@srowen
Copy link
Member

srowen commented Feb 9, 2016

merged to master

@asfgit asfgit closed this in 159198e Feb 9, 2016
markgrover added a commit to markgrover/spark that referenced this pull request Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants