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-25286][CORE] Removing the dangerous parmap #22292

Closed
wants to merge 3 commits into from

Conversation

MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Aug 30, 2018

What changes were proposed in this pull request?

I propose to remove one of parmap methods which accepts an execution context as a parameter. The method should be removed to eliminate any deadlocks that can occur if parmap is called recursively on thread pools restricted by size.

@MaxGekk
Copy link
Member Author

MaxGekk commented Aug 30, 2018

@zsxwing @gatorsmile Please, have a look at the PR.

Copy link
Member

@zsxwing zsxwing left a comment

Choose a reason for hiding this comment

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

LGTM except one nit

@@ -315,7 +315,9 @@ private[streaming] object FileBasedWriteAheadLog {
implicit val ec = executionContext
Copy link
Member

Choose a reason for hiding this comment

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

nit: this line is not needed

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

@SparkQA
Copy link

SparkQA commented Aug 31, 2018

Test build #95492 has finished for PR 22292 at commit 6fd4c00.

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

@kiszk
Copy link
Member

kiszk commented Aug 31, 2018

For future, let us keep a connection with #22233 that is the original one

@SparkQA
Copy link

SparkQA commented Aug 31, 2018

Test build #95526 has finished for PR 22292 at commit e868edb.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@MaxGekk
Copy link
Member Author

MaxGekk commented Aug 31, 2018

jenkins, retest this, please

@SparkQA
Copy link

SparkQA commented Aug 31, 2018

Test build #95531 has finished for PR 22292 at commit e868edb.

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

@gatorsmile
Copy link
Member

Thanks! Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants