Skip to content

Conversation

@mukulmurthy
Copy link
Contributor

What changes were proposed in this pull request?

Limit Thread Pool size in BlockManager Master and Slave endpoints.

Currently, BlockManagerMasterEndpoint and BlockManagerSlaveEndpoint both have thread pools with nearly unbounded (Integer.MAX_VALUE) numbers of threads. In certain cases, this can lead to driver OOM errors. This change limits the thread pools to 100 threads; this should not break any existing behavior because any tasks beyond that number will get queued.

How was this patch tested?

Manual testing

Please review http://spark.apache.org/contributing.html before opening a pull request.

@SparkQA
Copy link

SparkQA commented Aug 21, 2018

Test build #95057 has finished for PR 22176 at commit c2223c3.

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

@mukulmurthy
Copy link
Contributor Author

retest this please

@mukulmurthy
Copy link
Contributor Author

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Aug 22, 2018

Test build #95066 has finished for PR 22176 at commit c2223c3.

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

@mukulmurthy
Copy link
Contributor Author

@zsxwing , @JoshRosen , @marmbrus for review and merge.

@zsxwing
Copy link
Member

zsxwing commented Aug 22, 2018

LGTM. Merging to master.

@asfgit asfgit closed this in 68ec4d6 Aug 22, 2018
@mukulmurthy mukulmurthy deleted the 25181-threads branch August 22, 2018 17:55
@markhamstra
Copy link
Contributor

@zsxwing we really should have considered whether this should be a configuration variable instead of a fixed number of threads in any environment.

@zsxwing
Copy link
Member

zsxwing commented Aug 22, 2018

@markhamstra That's a good point. However, since this is just following our current codes if you check the usages of newDaemonCachedThreadPool, and the changes here should be safe considering how we use them, I don't want to block this PR for this reason. We can open a new ticket to add configurations if that's necessary.

@markhamstra
Copy link
Contributor

Yes, this is better than what we had, but maybe it can be better still.

otterc pushed a commit to linkedin/spark that referenced this pull request Mar 22, 2023
… Slave endpoints

Limit Thread Pool size in BlockManager Master and Slave endpoints.

Currently, BlockManagerMasterEndpoint and BlockManagerSlaveEndpoint both have thread pools with nearly unbounded (Integer.MAX_VALUE) numbers of threads. In certain cases, this can lead to driver OOM errors. This change limits the thread pools to 100 threads; this should not break any existing behavior because any tasks beyond that number will get queued.

Manual testing

Please review http://spark.apache.org/contributing.html before opening a pull request.

Closes apache#22176 from mukulmurthy/25181-threads.

Authored-by: Mukul Murthy <[email protected]>
Signed-off-by: Shixiong Zhu <[email protected]>
(cherry picked from commit 68ec4d6)

RB=1413511
BUG=LIHADOOP-40158
G=superfriends-reviewers
R=fli,mshen,yezhou,edlu
A=edlu
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