Skip to content

[SPARK-25901][CORE] Use only one thread in BarrierTaskContext companion object #22912

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

Closed
wants to merge 3 commits into from

Conversation

yogeshg
Copy link
Contributor

@yogeshg yogeshg commented Oct 31, 2018

What changes were proposed in this pull request?

Now we use only one timer (and thus a backing thread) in BarrierTaskContext companion object, and the objects can add timerTasks to that timer.

How was this patch tested?

This was tested manually by generating logs and seeing that they look the same as ones before, namely, that is, a partition waiting on another partition for 5seconds generates 4-5 log messages when the frequency of logging is set to 1second.

@SparkQA
Copy link

SparkQA commented Oct 31, 2018

Test build #98334 has finished for PR 22912 at commit 5206397.

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

@yogeshg yogeshg changed the title [SPARK-25901] Use only one thread in BarrierTaskContext companion object [SPARK-25901][CORE] Use only one thread in BarrierTaskContext companion object Oct 31, 2018
@yogeshg
Copy link
Contributor Author

yogeshg commented Oct 31, 2018

In an offline discussion with @MrBago , we noted that there's at most as many (non-cancelled) timerTasks on the timer as there are slots. So, one thread for managing logging is probably fine, in fact if anything, we should also think about how we can just use the main thread. Also, this means that the timer.purge() call in the finally block is also O(n + log c) \in O(constant) where n is the total number of tasks and c is the number of cancelled tasks.

@SparkQA
Copy link

SparkQA commented Nov 1, 2018

Test build #98340 has finished for PR 22912 at commit e7ad8ab.

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

@SparkQA
Copy link

SparkQA commented Nov 1, 2018

Test build #98341 has finished for PR 22912 at commit 2f2de0b.

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

@HyukjinKwon
Copy link
Member

cc @jiangxb1987 and @mengxr

Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

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

LGTM

@jiangxb1987
Copy link
Contributor

Thanks, merging to master!

@asfgit asfgit closed this in 0e318ac Nov 3, 2018
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
…on object

## What changes were proposed in this pull request?

Now we use only one `timer` (and thus a backing thread) in `BarrierTaskContext` companion object, and the objects can add `timerTasks` to that `timer`.

## How was this patch tested?

This was tested manually by generating logs and seeing that they look the same as ones before, namely, that is, a partition waiting on another partition for 5seconds generates 4-5 log messages when the frequency of logging is set to 1second.

Closes apache#22912 from yogeshg/thread.

Authored-by: Yogesh Garg <1059168+yogeshg@users.noreply.github.com>
Signed-off-by: Xingbo Jiang <xingbo.jiang@databricks.com>
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.

4 participants